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

Framework: Use standard polyfill pattern for Element#closest polyfill #7159

Closed
aduth opened this issue Jun 5, 2018 · 3 comments · Fixed by #9750
Closed

Framework: Use standard polyfill pattern for Element#closest polyfill #7159

aduth opened this issue Jun 5, 2018 · 3 comments · Fixed by #9750
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Task Issues or PRs that have been broken down into an individual action to take

Comments

@aduth
Copy link
Member

aduth commented Jun 5, 2018

Related: #7033 (specifically overview at #7033 (review) )

We should consolidate our approaches to polyfilling missing behavior, mostly impacting Internet Explorer support. Our use of the element-closest module has proven difficult to maintain. The approach used in #7033 for polyfilling Node#contains is looking to be a better candidate for its maintenance and performance implications.

As part of this, we must determine a way to ensure these polyfills are loaded by all dependent scripts. Some ideas:

In that sense, and given that these are true progressive enhancements which do nothing if a browser is detected to support a given feature, I'd say: Load it everywhere; every WordPress admin screen; for any features we ever use.

#7033 (review)

we should probably not need to load it if:

  • There are zero scripts on a screen otherwise
  • We have some way of knowing / inferring that modern authoring practices are employed
    • Only those which are transpiled (from a build directory)?
    • Some other metric akin to @babel/preset-env@next's useBuiltIns: 'usage'?
    • Register scripts through a new abstracted registration function, e.g. wp_register_script_with_polyfill?

#7033 (comment)

There's a couple things I'd like to see, but not necessarily as part of these changes:

  • Avoid polyfills being considered as dependencies of specific scripts, unless we're certain we can maintain those dependencies.
    • There's nothing about wp-dom that depends on the Node#contains polyfill. Wherever we're using it should be the dependency
    • But then again, this highlights the bigger problem that this is a source of maintenance headaches and is likely not worth maintaining, vs. load always
  • Better names for the script enqueues, which avoids potential plugin incompatibilities. Probably something with a WP-specific prefix, e.g. wp-polyfill-node-contains

#7033 (review)

@aduth aduth added [Type] Task Issues or PRs that have been broken down into an individual action to take Framework Issues related to broader framework topics, especially as it relates to javascript labels Jun 5, 2018
@aduth
Copy link
Member Author

aduth commented Jun 19, 2018

Related Slack conversation (Core JS Meeting, 2018-06-19):

https://wordpress.slack.com/archives/C5UNMSU4R/p1529414641000073

(Link requires registration)

@cristianozanca
Copy link

Hi,
just found this error, using Gutenberg - Yoast - Woo Gutengerg Block plugins
Uncaught Error: only one instance of babel-polyfill is allowed http://wordpress.woo/wp-content/plugins/wordpress-seo/js/dist/commons-820.min.js?ver=8.2 line 1

@aduth
Copy link
Member Author

aduth commented Sep 14, 2018

Hi @altrovideo , thanks for the report. You can follow Yoast/wordpress-seo#10969 for Yoast's work on this. It appears to have been merged, so should be resolved in an upcoming release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants