Protect against template strings

[aleclarson]

Here's a real-world example causing some problems: https://github.com/jashkenas/coffeescript/blob/cbc695b83162c0ee0ffeb61308625f572ee42d6e/lib/coffeescript/coffeescript.js#L194

[zanona]

Can't we make it validate sourceMappingURL string only at the beginning of the line as it's supposed to be? Please see referenced issues, coming from AVA and then nyc. Thanks.

[aleclarson]

@zanona Yeah, that would be best. It's not mentioned in the spec that //# sourceMappingURL= must be at the beginning of the line, but I haven't seen any compilers not do that. And we can still allow whitespace in front, I guess.

Any thoughts, @thlorenz?

[thlorenz]

That'd be a breaking change, could we at least do something like where it can only have whitespaces in between the beginning of the line and the comment?
That'd allow things like ' //# sourceMappingUrl='.

[thlorenz]

Realizing that this wouldn't fix the coffee script problem :(

How about a option we pass, i.e. { strict: true } that will do what @zanona suggests?
That way we wouldn't break any existing libs as we'd set strict: false by default.

[zanona]

@thlorenz, the only problem is that currently there are quite a few packages depending on convert-source-map. I for one, came from ava and then nyc and here 😜.

So this would require all these packages to change their codebase in order to upgrade which I am not sure they are willing to do.

To be honest, I see this as a breaking-change, yes, but it's a bug fix as well, so strict in this case would be the right way of handling that which should always default to true and not false.

I really don't want to assume, but my initial thought is that most users are indeed only validating //# sourceMappingUrl= for its real purpose (converting source maps) and not using that string within code, otherwise they would probably have reported this as a bug, because it would not be expected.

What do you guys reckon?

[thlorenz]

I believe your assumption is correct, but still some modules may break with strict: true as default.
To avoid that we'd make it false and then the coffee-script people can bug the authors of other libs to add the strict: true flag.
I understand that's more work, but I'd favor that over breaking a bunch of modules.

It can have pretty far reaching effects since tons of modules use convert-source-map directly or indirectly. So we have some responsibility here.
However if enough collaborators think otherwise I'm open to follow your suggestion anyways @zanona .

Let's see if others chime in ....

[zanona]

@thlorenz, I completely understand and I believe we have good options.

next patch

Setting strict: true in order to get the intended behaviour (default is: false)

next major release

Adjust default strict option value to true since is really the expected behaviour

@novemberborn would you be happy with that so it's adjusted on nyc?
Perhaps you could let us know your thoughts on this as well?

refs: avajs/ava#1595, istanbuljs/nyc#726

[aleclarson]

I disagree with adding a strict option in the next patch. Just fix the template string case with #61, then add the breaking change in next major.

Then library authors don't need to opt-in to get the correct behavior for template strings, which is the real issue right now.

[aleclarson]

It's worth noting that commentRegex has a leading ^\s*.

Is there a reason mapFileCommentRegex doesn't already have that, or just a slip up?

Since inline source maps already have the behavior that @zanona is suggesting, perhaps it makes sense to patch the same behavior into mapFileCommentRegex.

[thlorenz]

commentRegex has ^\s* so it would still break the coffee-script case for template strings.

I have no problem adapting the mapFileCommentRegex to behave like that, but I don't want to remove the optional whitespace chars since that could break people.

Then library authors don't need to opt-in to get the correct behavior
Correct or not correct there might be source maps out there that look like the below

some code here

  //# sourceMap...

(i.e. they have a space at the beginning of the source map line).
I'd like to avoid breaking these cases.

[aleclarson]

@thlorenz Those cases would not break by merging #61 (what I suggested we do). If we patch in a strict option (that defaults to false), the template string case won't be fixed without effort by the library author.

commentRegex has ^\s* so it would still break the coffee-script case for template strings.

I think you misunderstood me. I was pointing out that commentRegex already skips comments with non-spaces in front. So the current behavior for inline source maps is already different from sourceMappingURL comments. I still think patching #61 is the best option, and adding ^\s* to mapFileCommentRegex should come in a major release.

[thlorenz]

Ah, ok I get it now. Yeah mapFileCommentRegex are less forgiving (they were added much later than the souceMappingURL).
So I wouldn't change anything about that behavior as they are already strict and things work like that.

However let's keep the commentRegEx non-strict by default (is my suggestion).

We can merge #61 since that seems fine to me and think about the strict flag out side of that PR.

[thlorenz]

If I understand correctly #61 will only fix one liner template strings, but not things like

`
my string with some info about how to use
   //#sourceMapComment
`

at any rate there seems to be no simple fix either way for the below case:

`
my string with some info about how to use
//#sourceMapComment
`

[aleclarson]

Yeah, I don't think RegExp can do look-behind on line before first matched line. I highly doubt anyone is doing such a thing, and it would be ill-advised to do so. Let's merge #61 and publish v1.5.1 asap. 😄

[zanona]

Guys, do we need any other info or are relying on something else to make this happen?

[aleclarson]

@zanona It's in v1.5.1 as 46a6c17