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

Tiled gallery block: add babel-polyfill #31228

Closed
wants to merge 2 commits into from

Conversation

simison
Copy link
Member

@simison simison commented Mar 5, 2019

Changes proposed in this Pull Request

Testing instructions

  • Spin up jurassic ninja with this branch gutenpack-jn
  • Add Tiled gallery to a page where no other blocks are not present (to ensure you're not loading wp-polyfill to cover the issue,
  • open the saved page in IE11 and observe no more fatal errors in debugger

Fixes #31224

@simison simison added [Type] Bug When a feature is broken and / or not performing as intended [Goal] Gutenberg Working towards full integration with Gutenberg [Block] Tiled Gallery labels Mar 5, 2019
@matticbot
Copy link
Contributor

@simison simison requested a review from a team March 5, 2019 20:30
@simison simison self-assigned this Mar 5, 2019
@blowery
Copy link
Contributor

blowery commented Mar 5, 2019

We should probably consider using useBuiltIns: usage for @babel/preset-env here instead of entry. You don't really need the full polyfill set for the tiled gallery, probably just a couple things.

@sirreal sirreal added this to the Jetpack 7.1.1 milestone Mar 5, 2019
Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

Thanks for this, left a few notes I'd like to consider before merging.

/**
* External dependencies
*/
import '@babel/polyfill';
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to compare the size change for the view script with this addition.

If we depend on wp-polyfill we can share the cost of the dependency with any other code that may be using it.

I'd like to consider that before moving ahead with this.

Copy link
Member Author

@simison simison Mar 5, 2019

Choose a reason for hiding this comment

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

wp-polyfill (v7.0.0) 30.7 KB minified

view.js when building minified production versions:

  • without polyfill 7.96 KiB
  • with polyfill 87.6 KiB

So looks like we're better enqueing wp-polyfill as an immediate fix and then later on we'll fix the builder.

Copy link
Member Author

@simison simison Mar 5, 2019

Choose a reason for hiding this comment

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

So looks like we're better enqueing wp-polyfill as an immediate fix

Automattic/jetpack#11487

@simison
Copy link
Member Author

simison commented Mar 5, 2019

FYI @jeffersonrabb and @ockham since Slideshow block also includes polyfill:

We've meant to look into this — ideally SDK builder would just take care of this and polyfill things that are only needed. Calypso would also benefit instead of adding it manually?

@sirreal
Copy link
Member

sirreal commented Mar 5, 2019

ideally SDK builder would just take care of this

Yes! This is handled for us in Calypso and I'd expect the build tools to provide the same functionality.

@simison
Copy link
Member Author

simison commented Mar 5, 2019

Closing this as we're going ahead with Automattic/jetpack#11487 instead.

Build tooling polyfill follow-ups: #31231

@simison simison closed this Mar 5, 2019
@simison simison deleted the update/tiled-gallery-polyfill branch March 5, 2019 22:46
jeherve pushed a commit to Automattic/jetpack that referenced this pull request Mar 6, 2019
The previous version of Tiled gallery had `wp-i18n` as a dependency which depends on `wp-polyfill` (https://github.com/WordPress/WordPress/blob/029fcf7791e425303d14328471ed61e9f0ecc1e1/wp-includes/script-loader.php#L424).

We removed view side deps in #11326

By adding `wp-polyfill`, we ensure that `isNaN`, `Array.from` and other methods that need polyfilling to work in IE11.

Editor side dependency isn't _really_ needed here since many other dependencies pull in wp-polyfill as well, but seems like it's good to be explicit?

This fix is an alternative to Automattic/wp-calypso#31228 because of size considerations: Automattic/wp-calypso#31228 (comment):
>wp-polyfill (v7.0.0) 30.7 KB minified
>
>view.js when building minified production versions from Automattic/wp-calypso#31228:
>- without polyfill 7.96 KiB
>- with polyfill 87.6 KiB


#### Changes proposed in this Pull Request:
- Add `wp-polyfill` as a dependency for tiled gallery block view side and editor side.

#### Testing instructions:

- Spin up jurassic ninja with this branch gutenpack-jn
- Add Tiled gallery to a page where no other blocks are not present (to ensure you're not loading `wp-polyfill` via other sources)
- open the saved page in IE11 and observe no more  fatal errors in debugger

Fixes Automattic/wp-calypso#31224

#### Proposed changelog entry for your changes:
Fix regression that caused Tiled Gallery not function in Internet Explorer 11 browsers anymore.
jeherve pushed a commit to Automattic/jetpack that referenced this pull request Mar 6, 2019
The previous version of Tiled gallery had `wp-i18n` as a dependency which depends on `wp-polyfill` (https://github.com/WordPress/WordPress/blob/029fcf7791e425303d14328471ed61e9f0ecc1e1/wp-includes/script-loader.php#L424).

We removed view side deps in #11326

By adding `wp-polyfill`, we ensure that `isNaN`, `Array.from` and other methods that need polyfilling to work in IE11.

Editor side dependency isn't _really_ needed here since many other dependencies pull in wp-polyfill as well, but seems like it's good to be explicit?

This fix is an alternative to Automattic/wp-calypso#31228 because of size considerations: Automattic/wp-calypso#31228 (comment):
>wp-polyfill (v7.0.0) 30.7 KB minified
>
>view.js when building minified production versions from Automattic/wp-calypso#31228:
>- without polyfill 7.96 KiB
>- with polyfill 87.6 KiB


#### Changes proposed in this Pull Request:
- Add `wp-polyfill` as a dependency for tiled gallery block view side and editor side.

#### Testing instructions:

- Spin up jurassic ninja with this branch gutenpack-jn
- Add Tiled gallery to a page where no other blocks are not present (to ensure you're not loading `wp-polyfill` via other sources)
- open the saved page in IE11 and observe no more  fatal errors in debugger

Fixes Automattic/wp-calypso#31224

#### Proposed changelog entry for your changes:
Fix regression that caused Tiled Gallery not function in Internet Explorer 11 browsers anymore.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Tiled Gallery DO NOT MERGE [Goal] Gutenberg Working towards full integration with Gutenberg [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants