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: Add wp-polyfill as central polyfill #9794

Merged
merged 1 commit into from
Sep 12, 2018
Merged

Conversation

aduth
Copy link
Member

@aduth aduth commented Sep 11, 2018

Related (will require changes to): #9750

This pull request seeks to refactor our polyfills, introducing a new central wp-polyfill handle which includes polyfills for both JavaScript language features (Promise, etc) and modern browser features (currently window.fetch, Node#contains, FormData, soon Element#closest).

Implementation notes:

It does not remove or deprecate wp-polyfill-ecmascript, but rather acts as a superset, where the intended contract of "ecmascript" would be for language features, regardless of whether the underlying babel-polyfill / core-js provides browser polyfills.

Testing instructions:

Verify there are no regressions in the behavior of polyfills, particularly in Internet Explorer 11.

Note that there is currently a separate issue with saving in IE11, not addressed here.

@aduth aduth added Framework Issues related to broader framework topics, especially as it relates to javascript Browser Issues Issues or PRs that are related to browser specific problems labels Sep 11, 2018
@aduth aduth requested a review from atimmer September 11, 2018 14:43
@@ -99,66 +99,84 @@ function gutenberg_register_scripts_and_styles() {

register_tinymce_scripts();

wp_register_script(
'wp-polyfill',
null,
Copy link
Contributor

Choose a reason for hiding this comment

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

In one of my plugins, I was using this technique but I recall it was creating an issue for the script loading order. I can't reproduce thought here but thought I'd mention.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, okay, it's good to note. I've not encountered an issue like that which you describe. I did find that wp_add_inline_script doesn't work with the null $src, which is why I've opted for the equivalent wp_script_add_data below.

array(
'\'fetch\' in window' => 'wp-polyfill-fetch',
'document.contains' => 'wp-polyfill-node-contains',
'window.FormData && window.FormData.prototype.keys' => 'wp-polyfill-formdata',
Copy link
Contributor

Choose a reason for hiding this comment

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

Weren't you planning to add element closest or are you leaving this for a separate PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Weren't you planning to add element closest or are you leaving this for a separate PR?

If we can review and merge #9750 before this one, I can rebase and add it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the opposite is simpler right?

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@afercia
Copy link
Contributor

afercia commented Sep 12, 2018

Testing on IE 11, here's what I see:

screenshot 80 only gutenberg

first one wp-polyfill-fetch.6bd312cf.js (78,1) seems it's about the line 78
export function Headers(headers) { ...

I guess IE 11 doesn't know what export is. Really not my area of expertise, but shouldn't these files be transpiled before being served to IE 11?

Not sure about the error related to fetch. Expanding the error details in the IE 11 console doesn't give any useful hint, but I guess it's related to the above error.

@aduth
Copy link
Member Author

aduth commented Sep 12, 2018

@afercia The issue you're seeing there is addressed by #9795, which has since been merged. If you git fetch origin and git rebase origin/master your local copy of this branch, the issue should go away. I'm verifying this myself at the moment too.

@afercia
Copy link
Contributor

afercia commented Sep 12, 2018

@aduth thanks will check. Note: I've also seen Object doesn't support property or method 'includes' when activating Yoast SEO and the Classic Editor plugin, it may be unrelated though. Will investigate.

@aduth
Copy link
Member Author

aduth commented Sep 12, 2018

In my testing after rebasing with the included #9795 fix, everything here works as expected in IE11.

@afercia
Copy link
Contributor

afercia commented Sep 12, 2018

Yup I don't see the error related to fetch any longer.

@aduth aduth merged commit 6d7b5a7 into master Sep 12, 2018
@aduth aduth deleted the refactor/polyfills branch September 12, 2018 13:38
@mtias mtias added this to the 3.9 milestone Sep 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Browser Issues Issues or PRs that are related to browser specific problems Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants