Skip to content

Avoid source-map-loader matches incorrect source map url #181

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

Closed
wants to merge 1 commit into from

Conversation

zuohaocheng
Copy link

What kind of change does this PR introduce?

bugfix

Did you add tests for your changes?
No. There is no test in the repository.

If relevant, did you update the README?
Not relevant.

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.

This PR provide a workaround on the issue.

Does this PR introduce a breaking change?
No.

Other information

@jsf-clabot
Copy link

jsf-clabot commented Mar 3, 2017

CLA assistant check
All committers have signed the CLA.

@zuohaocheng
Copy link
Author

It seems there is something wrong in the CI config, which thinks the project is in Ruby.

May anyone kindly take a look?

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 There is no .travis.yml file specified for style-loader atm, but Travis CI is used on organisation level. Travis defaults to ruby ;)

@webpack-contrib/org-maintainers A second pair of eyes needed :)

@ekulabuhov
Copy link
Member

IMO, this should be fixed in source-map-loader instead.

@bebraw
Copy link
Contributor

bebraw commented Mar 6, 2017

IMO, this should be fixed in source-map-loader instead.

Yeah, that could be a better direction. 👍

@michael-ciniawsky
Copy link
Member

Related #132

@michael-ciniawsky
Copy link
Member

@bebraw @ekulabuhov kk 👍

@zuohaocheng
Copy link
Author

@ekulabuhov @bebraw @michael-ciniawsky Thanks for review. Actually I'm working on fix in source-map-loader, and it should be done soon.

Meanwhile, it would still be nice to have a workaround here, to keep it from confusing other non-standard compliant consumers, like mentioned in #132

@michael-ciniawsky
Copy link
Member

@zuohaocheng How far from the finishing line is your source-map-loader fix? I promise to keep track of it and update here approbately (asap), if your/a fix is provided in source-map-loader, but would like to avoid any temporarily changes if possible 🙃

@michael-ciniawsky
Copy link
Member

Also please rebase, so added test in #176 can run at least

@zuohaocheng
Copy link
Author

@michael-ciniawsky I've rebased and CI passed, thanks for reminding.

I'm expecting to finish work on source-map-loader in a day or two, and will let you know when PR created there.

@joshwiens
Copy link
Member

Given this has been fixed in webpack-contrib/source-map-loader#31 this should / can be closed now yes?

@michael-ciniawsky
Copy link
Member

I'm not 💯, but hopefully yes :D @zuohaocheng Can you confirm? I will also take a look into it later

@zuohaocheng
Copy link
Author

@d3viant0ne @michael-ciniawsky Yes it is fixed in source-map-loader. I think it is ok for my case - though I'm still a bit worrying about other non-standard consumers.

@joshwiens
Copy link
Member

@bebraw @ekulabuhov - Personal opinion here is to contend with edge cases as the arise and not add a workaround for a non-existant problem which may have unintended side effects.

@bebraw
Copy link
Contributor

bebraw commented Mar 12, 2017

Yeah, maybe close this one given source-map-loader was fixed. If edge cases come up, fix then without trying to pre-empt problems that might not exist.

I think the bigger problem is that there's no standard test suite for testing anything source map related. Though tackling that might be more up to standards bodies and browser authors than webpack. 👍

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

Successfully merging this pull request may close these issues.

6 participants