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.

Sign up for GitHub

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

Jump to bottom

Use JsonpMainTemplatePlugin.hooks instead of MainTemplate#hooks #7673

Merged
merged 2 commits into from
Jul 9, 2018
Merged

Use JsonpMainTemplatePlugin.hooks instead of MainTemplate#hooks #7673

merged 2 commits into from
Jul 9, 2018

Conversation

ooflorent
Copy link
Member

Refactor JsonpMainTemplatePlugin plugin to change how hooks are installed into MainTemplate.
cc @jantimon

What kind of change does this PR introduce?

refactoring

Did you add tests for your changes?

no

Does this PR introduce a breaking change?

yes

What needs to be documented once your changes are merged?

Maybe JsonpMainTemplatePlugin.hooks.

@webpack-bot
Copy link
Contributor

For maintainers only:

  • This need to be documented (issue in webpack/webpack.js.org will be filed when merged)

@ooflorent ooflorent force-pushed the refactor/jsonp_hooks branch from b7de681 to 8e456f5 Compare July 6, 2018 11:19
@jantimon
Copy link
Contributor

jantimon commented Jul 6, 2018

@sokra @ooflorent I really like that approach and will also use it for the html-webpack-plugin.
Just one thing - some Plugins might provide a hooks object but others will require a context argument.

Imho we should prefix get functions e.g.: getHooks to be more consistent.

JsonpMainTemplatePlugin.getHooks(
vs
compilation.hooks

This was referenced Jul 6, 2018
Merged
Merged
sokra
sokra requested changes Jul 8, 2018
View reviewed changes
Copy link
Member

@sokra sokra left a comment

Choose a reason for hiding this comment

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

@jantimon renaming sounds good to me

lib/web/JsonpMainTemplatePlugin.js Outdated
class JsonpMainTemplatePlugin {
static hooks(mainTemplate) {
Copy link
Member

Choose a reason for hiding this comment

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

hooks -> getHooks

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright. I’ll do it in the other PR also

@sokra
Copy link
Member

sokra commented Jul 8, 2018

Should we add a instanceof MainTemplate check? It's very easy to do it wrong, i. e. JsonpMainTemplatePlugin.getHooks(compilation). This doesn't throw, but also don't work as expected. A instanceof MainTemplate could help by throwing an error.

@webpack-bot
Copy link
Contributor

@ooflorent Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@sokra Please review the new changes.

@ooflorent ooflorent force-pushed the refactor/jsonp_hooks branch from 1247090 to c9bd29c Compare July 9, 2018 06:45
@webpack-bot
Copy link
Contributor

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@sokra sokra merged commit 0c5433c into next Jul 9, 2018
@sokra sokra deleted the refactor/jsonp_hooks branch July 9, 2018 11:19
@sokra
Copy link
Member

sokra commented Jul 9, 2018

Thanks

@jantimon jantimon mentioned this pull request Sep 10, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sokra sokra sokra approved these changes

Assignees
No one assigned
Labels
PR: CI-ok PR: next PR: reviewed-approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ooflorent @webpack-bot @jantimon @sokra