Change RegExp to be less greedy (Fixes #87). #88
Conversation
| @@ -84,7 +84,7 @@ function retrieveSourceMapURL(source) { | |||
|
|
|||
| // Get the URL of the source map | |||
| fileData = retrieveFile(source); | |||
| var re = /\/\/[#@]\s*sourceMappingURL=([^'"]+)\s*$/mg; | |||
| var re = /(?:\/\/[@#][ \t]+sourceMappingURL=([^\s'"]+?)[ \t]*$)|(?:\/\*[@#][ \t]+sourceMappingURL=([^\*]+?)[ \t]*(?:\*\/){1}[ \t]*$)/mg; | |||
There was a problem hiding this comment.
Why so complex, wouldn't a simple non greedy \s*? at the end be enough?
There was a problem hiding this comment.
Because i took it from an other source map related project. It is in production, tested and it works.
There was a problem hiding this comment.
I am reluctant to had such a complex and not commented regexp…
From which project does it come from?
There was a problem hiding this comment.
It is from the convert-source-map project
https://www.npmjs.com/package/convert-source-map
The RegExp is on the following line:
https://github.com/thlorenz/convert-source-map/blob/master/index.js#L8
There was a problem hiding this comment.
It is already better with their comment above the regexp but I don't understand it fully, especially the {1}…
I wonder if this regexp should not be in this own package like ip-regex.
Anyway, are you sure all tests pass, even those in browsers?
There was a problem hiding this comment.
Yes, except the browserify one but that one was already broken.
There was a problem hiding this comment.
For the syntax regarding {1} see the Quantifiers table (row x{n}) at https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp
There was a problem hiding this comment.
I know, and for me {1} is useless as it is the default behaviour to match one instance…
There was a problem hiding this comment.
I ran the tests from convert-source-map without the {1} and it seems non-significant.
Please let me know if you plan on merging this (without or without the {1}) because for me this is a blocking issue otherwise i need to start looking for an alternative.
There was a problem hiding this comment.
I can merge this but only @evanw is able to publish a new version.
Before merging this, please remove the {1} and add the comment above the regex.
|
Also, why have you changed all the tests? |
|
So it is now possible to execute the tests on Windows. |
|
I undertand but it should be in a separate pull request. |
|
Partly reverted the test changes and created an issue #89 for it. |
|
Squashes your commits and we good to go. |
Change RegExp to be less greedy (Fixes #87).
|
Merged, you can now use it by installing the package from github ( |
Change RegExp to be less greedy (Fixes #87). Add test and allow tests to run (and pass) on Windows.