-
-
Notifications
You must be signed in to change notification settings - Fork 83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Load source map only from last directive #31
Conversation
index.js
Outdated
@@ -22,6 +23,7 @@ module.exports = function(input, inputMap) { | |||
var addDependency = this.addDependency; | |||
var emitWarning = this.emitWarning || function() {}; | |||
var match = input.match(regex1) || input.match(regex2); | |||
console.log(match); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the debug statement
Also, please rebase with current master to pick up the Travis build. |
0ce1def
to
5b01789
Compare
@d3viant0ne Thanks for reviewing, I've removed the line and rebased. There was an issue on test: The line ends on Windows is |
Just gave it a quick triage, we will give this a good look in the morning. I've linked the two issues and we will be sure to coordinate this with the associated PR in style-loader. |
test/index.test.js
Outdated
@@ -33,7 +33,7 @@ function execLoader(filename, callback) { | |||
return this.callback; | |||
} | |||
}; | |||
var res = loader.call(context, fs.readFileSync(filename, "utf-8")); | |||
var res = loader.call(context, fs.readFileSync(filename, "utf-8").replace(/\r/g, '')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Split in two lines: (e.g. var res = ... <newline> res = res.replace(/\r/g, '')
and add a comment explaining the addition.
Would it work if you only checked the last line? If I understand correctly current regex is checking the whole file. Great PR 👍 |
@ekulabuhov Thanks and updated accordingly. I'm afraid that some non-standard source inputs would be broken, if checking only the last line for source map directive. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zuohaocheng 👍 Thx
@d3viant0ne |
What kind of change does this PR introduce?
bugfix
Did you add tests for your changes?
Yes.
Summary
There is a library bundled by webpack with
style-loader
. A consumer tries to load it through webpack withsource-map-loader
, but it fails to build.The root cause is
source-map-loader
incorrectly thinks the string literal"\n/*# sourceMappingURL=data:application/json;base64,"
is a directive for source map, it tries to find the source map here(while the actual source map directive is in the end of file), and fails.Per source map spec, the directive should appear at the last line of js file. Thus it makes sense to use last match as the one we read source map from.
Does this PR introduce a breaking change?
No.
Other information
Relates to: webpack-contrib/style-loader#181