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

Load the import map polyfill only when there is an import map #56699

Merged
merged 3 commits into from
Dec 4, 2023

Conversation

luisherranz
Copy link
Member

What?

Load the import map polyfill only when there is an actual import map.

Why?

Because if there is no import map, it's not needed.

How?

By checking if the import map is empty before adding it to the HTML.

I also tried to load the polyfill conditionally by using this logic, but in my testing with iOS Safari 16.0 it didn't work.

<script>
if (!(HTMLScriptElement.supports && HTMLScriptElement.supports("importmap"))) {
  const importMapPolyfill = document.createElement("script");
  importMapPolyfill.async = true;
  importMapPolyfill.src = "URL-TO-POLYFILL";
  document.head.appendChild(importMapPolyfill);
}
</script>

This is the related issue in the es-module-shims repository:

I'll investigate why it didn't work. For now, I think this fix improves the current behavior.

Testing Instructions

  • Go to a page that has interactive blocks.
  • Check that the import map polyfill was added.
  • Go to a page that doesn't have interactive blocks.
  • Check that the import map polyfill was not added.

Also, in not supported browsers (Safari < 16.3):

  • Go to a page that has interactive blocks.
  • Check that they keep working.

@luisherranz luisherranz added [Type] Performance Related to performance efforts [Feature] Script Modules API Related to the Script Modules API that adds support for native ES modules and import maps labels Nov 30, 2023
@luisherranz luisherranz self-assigned this Nov 30, 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/interactivity-api/modules.php
❔ lib/experimental/modules/class-gutenberg-modules.php

@luisherranz
Copy link
Member Author

This didn't improve the LCP metrics, so I'll do more tests.

(index) 261dd1851b15ca33ec5cf73a3747e921672b2698 trunk
timeToFirstByte '52.5 ms' '51.95 ms'
largestContentfulPaint '103.2 ms' '104.25 ms'
lcpMinusTtfb '50.45 ms' '51.2 ms'

@youknowriad
Copy link
Contributor

The regression was close to 5%. and the margin of error can reach 5% as well in a single run but over multiple runs you can clearly see the trend.

So this PR shows 1% diff, best is to try to run again to confirm whether the numbers are consistent. So yeah it's most likely not reverting the regression entirely/yet.

@luisherranz
Copy link
Member Author

These are the results without the polyfill:

(index) de9132c0e714d8664b3a8bb2d1f1d9bc6ceb2a9e trunk
timeToFirstByte '49.8 ms' '50.1 ms'
largestContentfulPaint '101.65 ms' '100.95 ms'
lcpMinusTtfb '51.75 ms' '49.7 ms'

@luisherranz
Copy link
Member Author

If the polyfill is not the problem, I need to dig deeper to find out what's going on, because I can't imagine modules being slower than deferred scripts.

Copy link

github-actions bot commented Dec 4, 2023

Flaky tests detected in 063c18d.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7085116154
📝 Reported issues:

@luisherranz
Copy link
Member Author

Even though this doesn't fix the LCP issue, we should merge it because it improves the current logic.

I'll keep investigating the LCP issue in other PRs.

@luisherranz luisherranz marked this pull request as ready for review December 4, 2023 10:49
Copy link
Contributor

@cbravobernal cbravobernal left a comment

Choose a reason for hiding this comment

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

Tested and working as described. LGTM

@cbravobernal cbravobernal merged commit 8c4f435 into trunk Dec 4, 2023
53 checks passed
@cbravobernal cbravobernal deleted the load-the-import-map-polyfill-only-when-needed branch December 4, 2023 12:30
@github-actions github-actions bot added this to the Gutenberg 17.3 milestone Dec 4, 2023
@youknowriad
Copy link
Contributor

I think for classic themes, this PR seem to have restored the previous numbers for LCP but it had no impact on block themes.

@luisherranz
Copy link
Member Author

luisherranz commented Dec 6, 2023

That's great news because it means it's the polyfill, and all we need to do is load it only when the browser doesn't support import maps, which I tried and didn't succeed, but should be technically possible.

@luisherranz
Copy link
Member Author

For reference, we'll work on a new build of es-module-shim that contains only the polyfill and it's loaded only when necessary:

@luisherranz
Copy link
Member Author

@gziolo made it work using document.write:

@youknowriad youknowriad added the Backported to WP Core Pull request that has been successfully merged into WP Core label Jan 31, 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] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants