Protect against template strings about convert-source-map HOT 16 CLOSED

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.

from convert-source-map.

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

from convert-source-map.

@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?

from convert-source-map.

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='.

from convert-source-map.

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.

from convert-source-map.

@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?

from convert-source-map.

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 ....

from convert-source-map.

@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

from convert-source-map.

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.

from convert-source-map.

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.

from convert-source-map.

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.

from convert-source-map.

@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.

from convert-source-map.

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.

from convert-source-map.

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
`

from convert-source-map.

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. 😄

from convert-source-map.

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

from convert-source-map.