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

Compat: Use core-js-url-browser for URL polyfill #20225

Merged
merged 4 commits into from
Mar 12, 2020
Merged

Conversation

aduth
Copy link
Member

@aduth aduth commented Feb 14, 2020

Previously: #19871
Related: https://github.com/Financial-Times/polyfill-library/issues/462, #20172 (comment), Trac#49360

This pull request seeks to resolve issues where the current polyfill for URL introduced in #19871 is ineffective:

The proposed changes improve the condition, and use core-js-url-polyfill in place of polyfill-library for the URL polyfill. This is a package I've published to NPM, and is a simple wrapper of core-js's URL polyfills. See Trac#49360 comment for more context on why a wrapper module is needed.

Testing Instructions:

NOTE: The following can't yet be tested, because there is an unrelated error in IE11 preventing the page from being loaded. (TBD PR)

Prerequisites:

  • Use Internet Explorer
  • Either run WordPress 5.3.x (or any version earlier than 5.4-beta / trunk), or comment this line in the local copy of your branch:
    • return;
    • This is needed because Gutenberg's polyfill script is not registered if there is already one registered for wp-polyfill-url. Since this was already patched to core in 5.4-beta, the changes would not otherwise take effect.

Test:

  1. Navigate to Posts > Add New
  2. Open F12 Developer Tools Console
  3. Observe success: new URL( 'http://example.com' );
  4. Observe thrown error: new URL( 'invalid' );

Screen Shot 2020-02-13 at 7 47 02 PM

@aduth aduth added [Type] Bug An existing feature does not function as intended Browser Issues Issues or PRs that are related to browser specific problems [Package] Url /packages/url labels Feb 14, 2020
@aduth aduth force-pushed the fix/ie-url-polyfill branch from 1f502c1 to 9fa86b9 Compare March 6, 2020 16:43
@github-actions
Copy link

github-actions bot commented Mar 6, 2020

Size Change: 0 B

Total Size: 864 kB

ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.01 kB 0 B
build/annotations/index.js 3.43 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/index.js 104 kB 0 B
build/block-editor/style-rtl.css 10.6 kB 0 B
build/block-editor/style.css 10.6 kB 0 B
build/block-library/editor-rtl.css 7.36 kB 0 B
build/block-library/editor.css 7.36 kB 0 B
build/block-library/index.js 115 kB 0 B
build/block-library/style-rtl.css 7.5 kB 0 B
build/block-library/style.css 7.51 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.7 kB 0 B
build/components/index.js 191 kB 0 B
build/components/style-rtl.css 15.5 kB 0 B
build/components/style.css 15.5 kB 0 B
build/compose/index.js 5.75 kB 0 B
build/core-data/index.js 10.6 kB 0 B
build/data-controls/index.js 1.03 kB 0 B
build/data/index.js 8.22 kB 0 B
build/date/index.js 5.36 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-post/index.js 91.3 kB 0 B
build/edit-post/style-rtl.css 8.64 kB 0 B
build/edit-post/style.css 8.64 kB 0 B
build/edit-site/index.js 4.64 kB 0 B
build/edit-site/style-rtl.css 2.48 kB 0 B
build/edit-site/style.css 2.48 kB 0 B
build/edit-widgets/index.js 4.44 kB 0 B
build/edit-widgets/style-rtl.css 2.58 kB 0 B
build/edit-widgets/style.css 2.58 kB 0 B
build/editor/editor-styles-rtl.css 381 B 0 B
build/editor/editor-styles.css 382 B 0 B
build/editor/index.js 43.8 kB 0 B
build/editor/style-rtl.css 3.98 kB 0 B
build/editor/style.css 3.98 kB 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 734 B 0 B
build/format-library/index.js 7.11 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 1.92 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.48 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.68 kB 0 B
build/list-reusable-blocks/index.js 2.99 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 4.85 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.01 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 780 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.3 kB 0 B
build/server-side-render/index.js 2.55 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@aduth
Copy link
Member Author

aduth commented Mar 6, 2020

Per #20628 (comment) , I've rebased the branch and updated script registrations in 9fa86b9. Specifically, calls to gutenberg_register_vendor_script have been moved into the gutenberg_register_vendor_scripts function, to ensure that those scripts are downloaded as part of the plugin build script. I've confirmed that with these changes, the two vendor scripts (+minified variant) are included in the plugin ZIP file.

There is one minor difference to note: Previously, the logic would avoid calling add_inline_script if it could infer that the script was registered by core (i.e. running WordPress 5.4.0+). With the new setup, it's not as simple to detect whether the script registration occurred in core or in gutenberg_register_vendor_script, so the condition was removed. In practice, this means there may be a duplicate inline script for wp-polyfill, but it should make no effective difference, since the second condition should be satisfied by the loading of the script of the first inline script if the polyfill was needed (i.e. still at most one polyfill script loaded).

cc @mcsf

@aduth
Copy link
Member Author

aduth commented Mar 6, 2020

Note that this is meant to complement the downstream core revision r47416.

@aduth aduth requested review from mcsf and jorgefilipecosta March 6, 2020 16:52
@aduth aduth merged commit 03960e6 into master Mar 12, 2020
@aduth aduth deleted the fix/ie-url-polyfill branch March 12, 2020 20:00
@github-actions github-actions bot added this to the Gutenberg 7.8 milestone Mar 12, 2020
@ellatrix ellatrix mentioned this pull request Jun 16, 2020
12 tasks
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 [Package] Url /packages/url [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant