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

Modules: Load the import map polyfill when needed #57256

Merged
merged 2 commits into from
Jan 8, 2024

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Dec 20, 2023

What?

Explores whether there is a simple way to address the issue raised by @felixarntz in #55942 (comment):

For the import maps polyfill, it looks like that JS file is always loaded? I was using latest Chrome and it loaded for me, which it probably shouldn't (since Chrome supports import maps). Is there a way to perform a quick support check with inline JS instead so that the polyfill can only be loaded as needed? It's over 12kB, so if we can avoid that for most sites, that would be a good win.

Why?

It should be easier to handle in WordPress core, but as a quick experiment I replicated how we handle conditional polyfills in the past:

https://github.com/WordPress/wordpress-develop/blob/1bca32e30ede3f8731290b052b35c431d091e2f7/src/wp-includes/script-loader.php#L204-L214

How?

I used the conditional check for feature detection included on MDN:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Modules#feature_detection

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

It works correctly with browsers that support import maps. I tested with Safari and Chrome:

import-map-polyfill.mov

@gziolo gziolo self-assigned this Dec 20, 2023
Copy link

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files
❔ lib/experimental/modules/class-gutenberg-modules.php

@gziolo gziolo mentioned this pull request Dec 20, 2023
6 tasks
@gziolo gziolo marked this pull request as ready for review December 20, 2023 12:08
@gziolo gziolo added the [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible label Dec 20, 2023
@gziolo
Copy link
Member Author

gziolo commented Dec 20, 2023

I only make it a normal pull request to be able to use Gutenberg PR Previewer powered by Playground. I'm not sure if that will help though.

@gziolo gziolo added the [Type] Experimental Experimental feature or API. label Dec 20, 2023
adamziel added a commit to WordPress/wordpress-playground that referenced this pull request Dec 20, 2023
Fixes an issue reported by @gziolo:

> WordPress/gutenberg#57256 - I can’t preview
this PR using Playground
https://playground.wordpress.net/gutenberg.html. What do I miss? All CI
checks are green.

The culprit was that GitHub hosts that artifact on another domain:
`productionresultssa0.blob.core.windows.net`. The current
`file_get_contents` call follows the redirection URL and resends the
same HTTP headers. That used to be fine, but now the windows.net host
rejects those headers.

This PR ensures we don't implicitly follow the 302 redirect, but parse
the target URL and send an explicit request with a fresh set of HTTP
headers. As a bonus, this makes the download much faster as we now use
the `streamHttpResponse` function which immediately passes the response
bytes back to the browser instead of buffering the entire response
first.

This PR also switches from HEAD-request-based PR validation to the same
Query params–based one as the wordpress.html previewer uses.

## Testing instructions

Try to preview the above PR in the Gutenberg PR previewer on
https://playground.wordpress.net/gutenberg.html and confirm that it
works now (this fix is already deployed).
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Thanks @gziolo, this looks great! Two small concerns below.

@gziolo
Copy link
Member Author

gziolo commented Dec 20, 2023

It works as expected with browsers that don't support import maps, like Firefox 107:

Screen.Recording.2023-12-20.at.22.56.48.mov

@felixarntz, I'll look tomorrow into how to optimize it based on your feedback.

Copy link
Member

@luisherranz luisherranz left a comment

Choose a reason for hiding this comment

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

Tested it in iOS, iPadOS and macOS versions with Safari 13, 14 and 15, and it works as expected.

I tested again the document.head.appendChild(importMapPolyfill) solution and that didn't work, so it seems like document.write is the key to making this work.

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Thanks @gziolo, LGTM!

gziolo and others added 2 commits January 4, 2024 14:10
@gziolo gziolo force-pushed the fix/conditional-import-map-polyfill branch from 051bae9 to 8f5e260 Compare January 4, 2024 13:11
@gziolo gziolo merged commit 80f8d9e into trunk Jan 8, 2024
55 checks passed
@gziolo gziolo deleted the fix/conditional-import-map-polyfill branch January 8, 2024 13:09
@gziolo gziolo added [Type] Enhancement A suggestion for improvement. and removed [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible [Type] Experimental Experimental feature or API. labels Jan 8, 2024
@github-actions github-actions bot added this to the Gutenberg 17.5 milestone Jan 8, 2024
@gziolo gziolo added the [Feature] Script Modules API Related to the Script Modules API that adds support for native ES modules and import maps label Jan 8, 2024
@Mamaduka
Copy link
Member

Mamaduka commented Jan 9, 2024

I think this PR affected Largest Contentful Paint (classic) and LCP - TTFB (classic) stats - https://www.codevitals.run/project/gutenberg.

@luisherranz
Copy link
Member

luisherranz commented Jan 9, 2024

That's weird, because even though adding scripts with document.write is blocking and synchronous, it should not affect the latest versions of the browsers, which I guess is what the CWV is using to test.

We could try adding async, but if LCP is affected, it probably means the polyfill is being loaded when it shouldn't.

Previous related conversations:

@Mamaduka
Copy link
Member

Mamaduka commented Jan 9, 2024

@luisherranz, this could be due to the WP hook changing from wp_footer to wp_head, but I've not tested this theory.

@luisherranz
Copy link
Member

Yeah, but theoretically, in modern browsers, all it adds is a script tag with an if that resolves to false:

<script>
  if ( ! ( HTMLScriptElement.supports && HTMLScriptElement.supports( "importmap" ) ) ) {
    // Nothing else to do, because the if is always false in all the modern browsers.
  }
</script>

So that shouldn't affect the LCP even if it's in the head.

@luisherranz
Copy link
Member

I'm not sure I'm looking in the correct place, but if I am, it seems like the performance tests are using the latest chromium version:

`bash -c "source $HOME/.nvm/nvm.sh && nvm install && npm ci && npx playwright install chromium --with-deps && npm run build:packages"`,

Theoretically, that version should not load the polyfill.

@Mamaduka
Copy link
Member

Mamaduka commented Jan 9, 2024

That's correct; perf tests use the latest browser versions provided for Playwright - https://github.com/microsoft/playwright/releases/tag/v1.39.0.

@luisherranz
Copy link
Member

Tested Chromium 119 installed and run via playwright 1.3.9 locally in a arm Mac and confirmed that it does not load the polyfill.

Screenshot 2024-01-09 at 14 05 40

@luisherranz
Copy link
Member

luisherranz commented Jan 9, 2024

Just to be sure, we are looking at these numbers, right?

https://github.com/WordPress/gutenberg/actions/runs/7409967406/job/20161251805?pr=57256

  1. front-end-block-theme
(index) f7afa64d69f90ac4494c3a0b49b73d7fe9f673ee trunk
timeToFirstByte '51.8 ms' '51.3 ms'
largestContentfulPaint '106.2 ms' '104.4 ms'
lcpMinusTtfb '52.8 ms' '52.5 ms'
  1. front-end-classic-theme
(index) f7afa64d69f90ac4494c3a0b49b73d7fe9f673ee trunk
timeToFirstByte '39.15 ms' '38.35 ms'
largestContentfulPaint '113.8 ms' '103.85 ms'
lcpMinusTtfb '73.9 ms' '64.3 ms'

Maybe we should run them a few more times to see if the numbers stay still?

Any other ideas about what might be going on here?

cc: @youknowriad, @gziolo

@gziolo
Copy link
Member Author

gziolo commented Jan 10, 2024

That’s surprising results. It’s also unclear to me why it would impact negatively only the classic theme.

@gziolo
Copy link
Member Author

gziolo commented Jan 10, 2024

We could try moving the polyfill to the footer as it seems to work in my basic testing, but as Luis pointed out it's a simple JS check for over 90% of users.

@luisherranz
Copy link
Member

luisherranz commented Jan 13, 2024

Maybe we should run them a few more times to see if the numbers stay still?

Now that we've got a few more PRs after this one, it's clear that it affected the LCP (https://www.codevitals.run/project/gutenberg):

Screenshot 2024-01-13 at 11 57 47

As it still doesn't make sense that a single script with an if that evaluates to false is causing a performance issue, one test we could do to see if the browser that is used in the CI supports import maps or not is replacing the document.write() with a very expensive and blocking JS call. If that clearly increases the LCP, it means the if is not evaluating to false.

@getdave getdave added the Needs PHP backport Needs PHP backport to Core label Jan 15, 2024
@youknowriad youknowriad added Backported to WP Core Pull request that has been successfully merged into WP Core and removed Needs PHP backport Needs PHP backport to Core labels Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported to WP Core Pull request that has been successfully merged into WP Core [Feature] Script Modules API Related to the Script Modules API that adds support for native ES modules and import maps [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants