Skip to content

Preserve current wp-polyfill while adding a new, built version.#1405

Closed
desrosj wants to merge 1 commit intoWordPress:masterfrom
desrosj:trac/52941-alternate
Closed

Preserve current wp-polyfill while adding a new, built version.#1405
desrosj wants to merge 1 commit intoWordPress:masterfrom
desrosj:trac/52941-alternate

Conversation

@desrosj
Copy link
Copy Markdown
Member

@desrosj desrosj commented Jun 21, 2021

This is an alternate approach to fixing the regression in Core-52941.

In this PR:

  • The original wp-polyfill.js and wp-polyfill.min.js files are preserved to prevent 404 errors, just in case someone is targeting them directly.
  • A new wp-polyfill-built script handle is added to represent the new, custom built polyfill.
  • the regenerator-runtime script handle has been added representing the regenerator-runtime dependency. Scripts are built appropriately.
  • The wp-polyfill script handle is updated to be a psuedo script with wp-polyfill-built and regenerator-runtime as dependencies.

Trac ticket: https://core.trac.wordpress.org/ticket/52941.


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@desrosj desrosj self-assigned this Jun 21, 2021
@desrosj
Copy link
Copy Markdown
Member Author

desrosj commented Jun 21, 2021

There was also a newer version of the @babel/polyfill dependency available. I updated the version used to include the final released version of this archived package.

Copy link
Copy Markdown
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

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

Looks like it will work but a minor - non-blocking this close to beta3 - question inline.

}

$scripts->add( 'wp-polyfill', null, array( 'wp-polyfill' ) );
$scripts->add( 'wp-polyfill', null, array( 'regenerator-runtime', 'wp-polyfill-built' ) );
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a strong reason to avoid merging these two scripts in to one and retain the single script alias?

The though behind this question is two fold:

  • next time WP changes polyfill libraries, avoiding a -built-final-final-FINAL-2 type of situation
  • performance cost/benefit of sending one script vs two

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't mind having two scripts here. In the future, we might be able to drop regenerator-runtime for core scripts and make it opt-in for 3rd projects for backward compatibility. I'm not sure if that is going to be a simple task though.

'wp-polyfill-object-fit' => '2.3.5',
'wp-polyfill' => '7.4.4',
'wp-polyfill-built' => '6.2.0',
'wp-polyfill' => '7.10.1',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should it be 7.12.1 to align with @babel/polyfill?

Copy link
Copy Markdown
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Let's try it and see if it is compatible with all 3rd party scripts. Based on the feedback from some plugin authors it should be fine to replace the polyfill from Babel with the custom one without IE 11 support as long as we include regenerator-runtime. From the WP core perspective, regenerator-runtime is obsolete but maybe we can find a way in the future to make it opt-in for scripts that need it. We should encourage the community to start using the latest @wordpress/babel-preset-default so we can speed up the deprecation of regenerator-runtime.

@desrosj
Copy link
Copy Markdown
Member Author

desrosj commented Jun 23, 2021

Closing in favor of #1416, which was merged into Core.

@desrosj desrosj closed this Jun 23, 2021
@desrosj desrosj deleted the trac/52941-alternate branch June 23, 2021 00:33
@desrosj desrosj restored the trac/52941-alternate branch September 14, 2022 00:37
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.

3 participants