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

Remove including wp-polyfill with default create-block generation #35410

Closed
mkaz opened this issue Oct 6, 2021 · 5 comments · Fixed by #35436
Closed

Remove including wp-polyfill with default create-block generation #35410

mkaz opened this issue Oct 6, 2021 · 5 comments · Fixed by #35436
Assignees
Labels
[Package] Scripts /packages/scripts [Status] In Progress Tracking issues with work in progress [Tool] Create Block /packages/create-block

Comments

@mkaz
Copy link
Member

mkaz commented Oct 6, 2021

What problem does this address?

The default wp-scripts will include the polyfills dependency due to this line

With the support for IE11 removed, we no longer need the polyfill by default. It adds additional overhead with little gain, see #35038 for more details.

What is your proposed solution?

The easiest to solve would be to just remove the injectPolyfills: true from wp-scripts webpack config. I'm not sure if this would cause problems with backward compatibility or not.

An alternate would be to setup and environment variable to trigger polyfills, ideally we default it to false and then instruct people who need it to set to true. Suggestion: WP_WEBPACK_USE_POLYFILLS

We could then change the config line to: injectPolyfill: process.env.WP_WEBPACK_USE_POLYFILLS

Thoughts? cc: @gziolo @ryanwelcher @aristath

@aristath
Copy link
Member

aristath commented Oct 7, 2021

Removing the polyfills sounds reasonable to me.

An alternate would be to setup and environment variable to trigger polyfills, ideally we default it to false and then instruct people who need it to set to true. Suggestion: WP_WEBPACK_USE_POLYFILLS

Though this option is safer, maybe it's overkill? 🤔

@gziolo
Copy link
Member

gziolo commented Oct 7, 2021

That's a good question if we need the polyfill for plugins/blocks. We probably don't need it as it's going to be included anyway when setting any of the existing WordPress scripts handles that Gutenberg sends to the users.

@sgomes, what is your take on that? What would be the case when a plugin would need to explicitly ensure that the polyfill is included on the page?

@sgomes
Copy link
Contributor

sgomes commented Oct 7, 2021

Thanks for looping me in, @gziolo! This is all a policy and support decision, and I'm not too well-versed on how Core generally tends to approach these, but I'm more than happy to share my 0.02 money units.

I believe that giving control over polyfill loading to plugins would provide somewhat of a disincentive for the right course of action, which is to update a given plugin to native, modern JS — assuming the plugin even has a reason to care about polyfills in the first place. The option of not changing anything about the code or build process and forcing WP to maintain legacy behaviour will likely seem appealing to some time-strapped developers.

In general, a plugin's goal should be unrelated to anything around polyfills; plugins should be concerned with the WP functionality they provide, and not the browser environment it happens to run in. WP doesn't need these general polyfills and opts not to load them, so if a plugin itself needs or wants them, then I would argue that it's a plugin in need of some work to be compatible with newer versions of WP, or a plugin that's overly concerned with matters beyond its scope.

Besides, if a plugin really needs a specific polyfill (say, for something like Intersection Observers) it can always opt to ship it alongside its code. And it could even ship them all, if it's, say, a plugin specifically designed to maintain IE11 support. But giving plugins easy access to large spectrum polyfilling through Core functionality seems like opening the ecosystem up to potential problems in the future.

Hope this helps!

@gziolo
Copy link
Member

gziolo commented Oct 7, 2021

I read your response as - it's time to phase out polyfills from tools that the community uses 😄

The only risk I see is that some plugins might break when they update @wordpress/scripts to the version that doesn't include wp-polyfill as a script dependency. However, it's rather impossible in practice because all Gutenberg scripts request wp-polyfill so this polyfill should be included in WP-Admin anyway 🤷🏻

@sgomes
Copy link
Contributor

sgomes commented Oct 7, 2021

I read your response as - it's time to phase out polyfills from tools that the community uses 😄

That's a much less roundabout way of expressing my thoughts, yes! 😄

The only risk I see is that some plugins might break when they update @wordpress/scripts to the version that doesn't include wp-polyfill as a script dependency. However, it's rather impossible in practice because all Gutenberg scripts request wp-polyfill so this polyfill should be included in WP-Admin anyway 🤷🏻

Even if it were to happen a lot in practice, I think that would be fine. Situations like this are generally why projects have major versions, release notes, and, if we want to be really extensive and supportive, migration guides.

@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Scripts /packages/scripts [Status] In Progress Tracking issues with work in progress [Tool] Create Block /packages/create-block
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants