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

feat: Added ability to adjust plugin processAssets hook stage (#262) #263

Conversation

tomlagier
Copy link
Contributor

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers: #262

Description

This PR adds the ability to modify the processAssets stage that the WebpackManifestPlugin runs under. Currently, it is hard-coded to Infinity, which means that it will always run last in the processAssets stack. If we want to do some async behavior based on the output of the manifest, we need to schedule a hook later in the processAssets series.

This allows us to manually modify the stage the hook runs at by passing a processAssetsStage option. We can set it to, e.g. webpack.Compiler.PROCESS_ASSETS_STAGE_ANALYSE and then add our own plugin at PROCESS_ASSETS_STAGE_REPORT.

Copy link
Contributor Author

@tomlagier tomlagier left a comment

Choose a reason for hiding this comment

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

It looks like CI is failing because it wants security updates vis-a-vis webpack version. Let me know if you want me to fix that.

README.md Outdated
beforeEmit.tap('BatmanPlugin', (manifest) => {
return { ...manifest, name: 'hello' };
})
beforeEmit.tap("BatmanPlugin", (manifest) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is from the lint commit hook.

test/unit/options.js Show resolved Hide resolved
@shellscape
Copy link
Owner

@tomlagier thanks for opening this PR. I'm good with the concept with one caveat: I'd like to rename the option to assetHookStage. Aside from that, please update from upstream/master where we've fixed the dependency issues, and please resolve any conflicts. Once that's done, we'll merge and ship it.

@shellscape shellscape force-pushed the master branch 3 times, most recently from 4338db4 to a93ca8c Compare July 28, 2021 18:56
@tomlagier
Copy link
Contributor Author

Sounds good, will get those updates in today.

@tomlagier tomlagier force-pushed the tomlagier/add-processAssets-stage-option branch from 3abf43a to 300000b Compare August 8, 2021 20:07
@tomlagier
Copy link
Contributor Author

Updated the PR, but I was having some lint & TS errors in untouched files.

@shellscape
Copy link
Owner

That's strange. Just checked master and it's looking OK.

@shellscape shellscape force-pushed the tomlagier/add-processAssets-stage-option branch from 300000b to ba07ca9 Compare January 11, 2022 15:05
@shellscape
Copy link
Owner

This https://github.com/shellscape/webpack-manifest-plugin/runs/4777081149?check_suite_focus=true#step:11:92 is why CI was failing. I'll clean this up and push to your branch so this can be merged.

@shellscape shellscape merged commit 4072bfa into shellscape:master Jan 11, 2022
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.

2 participants