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

calypso-build: add webpack config for output.jsonpFunction #46252

Merged
merged 5 commits into from
Oct 21, 2020

Conversation

creativecoder
Copy link
Contributor

Changes proposed in this Pull Request

  • Add configuration for output.webpackjsonpFunction in the calypso-build package

The webpack docs state

If multiple webpack runtimes (from different compilations) are used on the same webpage, there is a risk of conflicts of on-demand chunks in the global namespace.

This seems to be a source of WordPress plugin conflicts, when multiple plugins use webpack to build js. For example: Automattic/jetpack#17289

Adding this config allows plugins that use calypso-build's webpack config, like Jetpack, to customize the webpack jsonp global variable name and avoid these conflicts.

Note that this config is changing to chunkLoadingGlobal in webpack 5, so we might want to address that in some way, as well.

Testing instructions

  • Add output-jsonp-function to a webpack build process that uses getWebpackConfig from calypso-build (Jetpack example: Automattic/jetpack@33815b4)
  • Check the output js, and see that window["webpackJsonp"] uses what you specified for the global variable

Required for fixing Automattic/jetpack#17289

@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

Copy link
Member

@jsnajdr jsnajdr left a comment

Choose a reason for hiding this comment

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

Nice! 👍

@@ -54,6 +54,7 @@ function getWebpackConfig(
'output-path': outputPath = path.join( process.cwd(), 'dist' ),
'output-filename': outputFilename = '[name].js',
'output-library-target': outputLibraryTarget = 'window',
'output-jsonp-function': outputJsonpFunction = 'webpackJsonp',
Copy link
Member

Choose a reason for hiding this comment

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

Maybe also add a line to the docblock comment few lines above? The existing documentation there is not terribly helpful though 🙁 (output-path sets output path and output-library-target sets output library target, who would have guessed that)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Done in b42eaaa

@creativecoder
Copy link
Contributor Author

@scinos @jsnajdr Thanks for the prompt review!

How do you recommend getting the calypso-build package updated so this change can be used externally? Okay to change package version in this PR (to 6.2.1) and update changelog.md, or should that be handled separately?

@jsnajdr
Copy link
Member

jsnajdr commented Oct 9, 2020

@creativecoder We usually do the release chores (changelog, version bump) as a separate PR. There is a P2 post about how to publish packages to NPM (with a video!) here: p4TIVU-9mL-p2. The NPM publishing docs in the Calypso repo also look reasonably up to date.

@creativecoder creativecoder merged commit cdc9683 into master Oct 21, 2020
@creativecoder creativecoder deleted the add/webpack-jsonpfunction-config branch October 21, 2020 23:56
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants