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

Add wp-polyfill as dependency to react #11916

Merged
merged 1 commit into from Nov 15, 2018

Conversation

Projects
None yet
4 participants
@IreneStr
Member

IreneStr commented Nov 15, 2018

Description

When investigating an IE11 issue in Yoast SEO, we ran into the issue that IE11 needs wp-polyfill to be registered before React and React DOM. It looks like this never leads to problems in Gutenberg itself because wp-polyfill has always already been loaded as a dependency of other dependencies. However, when plugins rely on Gutenberg's React, like we do, it does lead to issues in IE11.

How has this been tested?

IE11 test by @afercia in progress

Types of changes

Bugfix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
@@ -973,7 +973,8 @@ function gutenberg_register_vendor_scripts() {
gutenberg_register_vendor_script(
'react',
'https://unpkg.com/react@16.6.3/umd/react' . $react_suffix . '.js'
'https://unpkg.com/react@16.6.3/umd/react' . $react_suffix . '.js',
array( 'wp-polyfill' )

This comment has been minimized.

@youknowriad

youknowriad Nov 15, 2018

Contributor

Seems legit, should we do the same for react-dom?

This comment has been minimized.

@IreneStr

IreneStr Nov 15, 2018

Member

@youknowriad As far as in can see, react is a dependency of react-dom, so it will get wp-polyfill through react.

This comment has been minimized.

@youknowriad

youknowriad Nov 15, 2018

Contributor

Sounds good, If it's a direct dependency (which the React docs state I think) I do prefer explicitness.

This comment has been minimized.

@youknowriad

youknowriad Nov 15, 2018

Contributor

We should plan to backport this change into the 5.0 branch. cc @atimmer

This comment has been minimized.

@atimmer

atimmer Nov 15, 2018

Member

Putting it on my list.

@afercia

This comment has been minimized.

Contributor

afercia commented Nov 15, 2018

Tested and LGTM. However, I've noticed a couple warnings that happen on current master with only Gutenberg activated and no other plugins (see screenshots below). Will open a separate issue.

screenshot 142

screenshot 141

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 15, 2018

e2e tests are not happy though :)

@IreneStr

This comment has been minimized.

Member

IreneStr commented Nov 15, 2018

It looks like they're confused 😄

@youknowriad

LGTM 👍

@youknowriad youknowriad added this to the 4.4 milestone Nov 15, 2018

@youknowriad youknowriad merged commit cc91689 into master Nov 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the fix/add-wp-polyfill-as-dependency-to-react branch Nov 15, 2018

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment