Skip to content
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

Merged
merged 2 commits into from
Mar 7, 2017

Conversation

zuohaocheng
Copy link
Contributor

@zuohaocheng zuohaocheng commented Mar 7, 2017

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 with source-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

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);
Copy link
Member

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

@joshwiens
Copy link
Member

Also, please rebase with current master to pick up the Travis build.

@zuohaocheng zuohaocheng force-pushed the master branch 4 times, most recently from 0ce1def to 5b01789 Compare March 7, 2017 04:40
@zuohaocheng
Copy link
Contributor Author

@d3viant0ne Thanks for reviewing, I've removed the line and rebased.

There was an issue on test: The line ends on Windows is CRLF by git default setting, so the assertion couldn't pass. I added a step to remove all CRs in reading fixtures, making tests could be passed with CRLF.

@joshwiens joshwiens requested review from bebraw and ekulabuhov March 7, 2017 05:04
@joshwiens
Copy link
Member

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.

@@ -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, ''));
Copy link
Member

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.

@ekulabuhov
Copy link
Member

ekulabuhov commented Mar 7, 2017

Per source map spec, the directive should appear at the last line of js file

Would it work if you only checked the last line? If I understand correctly current regex is checking the whole file.

Great PR 👍

@zuohaocheng
Copy link
Contributor Author

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

Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zuohaocheng 👍 Thx

@michael-ciniawsky
Copy link
Member

@d3viant0ne :shipit: ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants