feat: Make comment RegExps non-greedy to prevent some max call stack errors

[Delagen]

No description provided.

[thlorenz]

This looks interesting and I'm fine with this in general as long as it doesn't break any other functionality.
It's been a while since you filed this (I'm sorry for the super long delay).
Is this still a problem for you or did the newer v8 version fix it?

[chpio]

Is this still a problem for you or did the newer v8 version fix it?

just got the

RangeError: Maximum call stack size exceeded
    at String.match (<anonymous>)
    at Object.exports.fromSource (.../node_modules/combine-source-map/node_modules/convert-source-map/index.js:122:19)
    at resolveMap (.../node_modules/combine-source-map/index.js:52:21)
    at Combiner.addFile (.../node_modules/combine-source-map/index.js:112:21)

exception. [email protected], [email protected] (1.8.0 is the current version)

wow the last release of combine-source-map is 5 years ago, and still rocking ancient deps.

[thlorenz]

Thanks @phated for cleaning this up. If no tests are failing we could merge.
Just have to be very careful whenever we change the regex as it could fail in unexpected ways.

Therefore if we do merge we need to publish as new major (I'll take care of that).

[phated]

Therefore if we do merge we need to publish as new major (I'll take care of that).

I agree! I was hoping to land #74 as a minor and then a few of the other open PRs as a major. Does that work for you @thlorenz ?

[thlorenz]

Yes that would work .. also now we got CI going again.

However the Node.js versions tested are super old now, we should probably test 0.14, 0.16, 0.18 instead.
So I'd like to get that in first, then merge #74, publish a minor and then worry about the regex PRs

[phated]

However the Node.js versions tested are super old now, we should probably test 0.14, 0.16, 0.18 instead.

We're actually testing pretty much every major version of node now: "0.10", "0.12", "4", "6", "8", "10", "12", "14", "16", "18"

[thlorenz]

Ah ok didn't see CI running those versions.
So not sure at this point cause this PR is at odds with #75.

It seems like they should be combined into one (possibly making the regex recommended in #75 non-greedy)?

Maybe @Delagen has an idea as well?

[phated]

Yeah, I am planning to merge this into master and then update #75 to take it into account.

[thlorenz]

LGTM to merge, so go ahead once we publish another minor with the old regex.
Should I go ahead and publish a minor at this point?

[phated]

Should I go ahead and publish a minor at this point?

Yep! I think the final minor is ready now.

[thlorenz]

OK, published 1.9 + merged CI PR. Now we can focos on that regex stuff :)