Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Packages: only add polyfills where needed #65292
Packages: only add polyfills where needed #65292
Changes from 13 commits
f0e3386
9a490fc
9d54993
74bab7c
1f7785b
5e926d3
0d03c67
a3d11f1
a1b0480
5a51225
d9ea190
061b7eb
91eb78d
28f1b5b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of making this configurable?
I understand this would require more setup to work correctly for third parties.
This would behave like
false
and apply the new behavior of searching for the comment string.Maybe this isn't necessary, but it would also avoid applying this for third parties when it's not desired or add a path to expose it to third parties in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to make changes, but I think we need to clarify the semantics here. As far as I can tell, what you're looking for is:
false
: Don't add polyfills, regardless of the presence of magic comments.true
: Add polyfills, regardless of the presence of magic comments.'auto'
: Add polyfills only when magic comments are present.Does that sounds about right?
To clarify, note that this is a separate build step to the one that actually adds the magic comments.
node ./bin/packages/build.js
builds the packages in situ, andwp-scripts build
is the one that takes each built package from its src directory, extracts the dependencies, and places the results in the top-levelbuild
dir.My reasoning for not adding a separate option was that if we didn't want auto polyfilling we probably wouldn't add magic comments in the first build step anyway (the
addPolyfillComments
option), but I'm happy to make changes if we feel it's still best to make this behaviour explicitly opt-in in DEWP as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% decided on this… let's leave it as-is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The strategy outlined seems good overall. The question is whether we really want to keep the differences between false and auto? The magic comment gets injected only with custom Babe code. In effect, in WP ecosystem there should never be a case where polyfill gets listed as a dependency outside of Gutenberg and Core. The only exception would be a project that bundles in a custom way WP packages but it's unlikely they would use the webpack plugin to generate asset files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said, should we ever enable magic comments by default in the default Babel preset that WordPress maintains?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Someone already filed Automattic/jetpack#39452 asking us to do it in Jetpack. 😀
I can't see any reason anyone would have the magic comment and not want the plugin to add the dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like a follow-up is very welcomed 👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could avoid having to preserve the comment through Terser by checking for the "wp:polyfill" comments in the
PROCESS_ASSETS_STAGE_OPTIMIZE_COMPATIBILITY
stage (rather thanPROCESS_ASSETS_STAGE_ANALYSE
as now).You could pass the "needs polyfill" flag between the two hooks with a map of some sort or by doing something like
compilation.updateAsset( filename, v => v, { wpPolyfill: flag } );
to set it and thenasset.info.wpPolyfill
to check it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would work. It would be a bit more complex, as it would require scanning all the chunks twice. We decided to keep the comment so we don't have to recalculate the hash generated for chunks. It sounds like a good follow up task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take that as an invitation. 🙂
#65582
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, I will have a look when I find some time in the next few days.