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 sourcemap in projects with code splitting #113

Merged
merged 4 commits into from Aug 29, 2019

Conversation

baileyid
Copy link
Contributor

@baileyid baileyid commented Aug 7, 2019

We’ve noticed an issue when using this plugin in projects with code splitting. Basically there are placeholders of SRI hashes being added to js binary, which are later replaced with real hash values. However, the placeholders in sourcemap files are not updated, so there could be a mismatch between the source code and sourcemap after where the variable of sriHashes is injected.
image

The cause is probably because the function of processChunk (which calls replaceAsset) is tapped to afterOptimizeAssets hook, while in webpack, the plugin that actually generates sourcemap is tapped to afterOptimizeChunkAssets hook. Probably in projects that don’t use code splitting, the two hooks happen at the same time, but in projects with code splitting, afterOptimizeChunkAssets happens before afterOptimizeAssets, so sriHashes replacement could break the sourcemap which was generated before that.

Changing the hook looks like fixing the issue.
image

@jscheid
Copy link
Collaborator

jscheid commented Aug 7, 2019

Hi, thanks for the PR and the analysis.

The build is failing, can you see why? I will also try and have a look.

It would be good to have a test case to demonstrate the need for this change, ie. one that fails before changing the hooks and succeeds after. This would ensure that source maps continue to be accurate in the future.

Relying on the order in which hooks are invoked can be fragile. Perhaps an alternate solution to this problem would be to ensure that placeholders have the same length as the replacement text. We could exploit the fact that all common hash functions produce a fixed-length result.

@baileyid
Copy link
Contributor Author

baileyid commented Aug 9, 2019

Thanks for the comments @jscheid ! Yeah, I agree that changing the placeholder may be a better idea. - I just generate a dummy sha in this case. Updated PR and added a test example. Please let me know what you think about it.

@baileyid
Copy link
Contributor Author

friendly ping @jscheid . Do you have time to take another look? Thanks!

Copy link
Collaborator

@jscheid jscheid left a comment

Choose a reason for hiding this comment

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

Good catch and great fix, thank you!

Thanks for your patience as well, the last few weeks have been quite busy for me.

I'm going to try and cut a release from this very soon, but there are a few other little things that have to go in. I'll comment on the PR when it's out, shouldn't be long.

@jscheid jscheid merged commit a211a15 into waysact:master Aug 29, 2019
@baileyid baileyid deleted the sourcemap-hook branch August 30, 2019 19:29
@baileyid baileyid restored the sourcemap-hook branch August 30, 2019 19:29
@jscheid
Copy link
Collaborator

jscheid commented Sep 2, 2019

@baileyid this is released in v1.3.3.

@baileyid
Copy link
Contributor Author

baileyid commented Sep 3, 2019

awesome. thank you!

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.

None yet

2 participants