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

Fix: use stringifyRequest instead of absolute paths in loader output #167

Merged
merged 1 commit into from
Dec 11, 2018

Conversation

redoPop
Copy link
Contributor

@redoPop redoPop commented Jun 28, 2018

Per Webpack's documentation on building loaders, the loader-utils package's stringifyRequest method should be used by loaders to avoid hash-breaking absolute paths. (See this article for more details on the broken hash problem.)

This PR applies Webpack's recommendation to handlebars-loader.

All tests pass. Since the stringifyRequest method is a part of the Webpack loader-utils package and acts as a drop-in replacement for JSON.stringify I didn't think additional tests seemed necessary, but please let me know if you'd like to see something added.

Per Webpack's documentation, the loader-utils package's stringifyRequest
method should be used by loaders to avoid hash-breaking absolute paths:

https://webpack.js.org/contribute/writing-a-loader/#absolute-paths
@NathanKleekamp
Copy link

Can we get an update on any progress getting this merged?

@NathanKleekamp
Copy link

@redoPop Have you reached out to the maintainers about getting this code reviewed and merged? Your fork has been working great for me, and I'd love to get it merged and start pointing to newer versions of this loader. Thanks for your contribution!!

@redoPop
Copy link
Contributor Author

redoPop commented Dec 4, 2018

I've not reached out to @pcardune except by filing this PR.

Paul: if there are any merge blockers, please let me know. It's not clear to me why Travis choked at the npm install stage; could you restart that build?

@pcardune
Copy link
Owner

I've been MIA for a bit. The build failed for unknown reasons. I just restarted it. Assuming everything passes, I'll go ahead and merge + make a release. Probably by Friday.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 89.157% when pulling fd9930b on redoPop:stringify-request into d70e93b on pcardune:master.

@pcardune pcardune merged commit d60c4ee into pcardune:master Dec 11, 2018
@pcardune
Copy link
Owner

version 1.7.1 has been released. Thanks for your patience.

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.

4 participants