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

Can we import module conditionally? #371

Open
bakura10 opened this issue Apr 29, 2023 · 9 comments
Open

Can we import module conditionally? #371

bakura10 opened this issue Apr 29, 2023 · 9 comments
Labels
enhancement New feature or request

Comments

@bakura10
Copy link

bakura10 commented Apr 29, 2023

Hi :),

This is not a bug but rather a question on how to embed the script. We are using this only for import map support. For now, we have followed the embed instructions to use an async script. As all browsers now support it natively, we would prefer to load it conditionally (we're not saving a lot, I know, but we are selling themes and a lot of our users are running their stores on PageSpeed, and those non-technical merchants often ask us how to "optimize their store by removing scripts" ; we therefore try to load as little JS as possible to avoid support debt).

I tried the following approach which works on all the iOS versions I have tested:

<script>
      if (!(HTMLScriptElement.supports && HTMLScriptElement.supports('importmap'))) {
        import('URL-TO-POLYFILL');
      }
    </script>

However, I am not 100% sure about possible consequences or potential timing issue. Considering this polyfill architecture, would this be as safe as embedding it using the async way?

Thanks.

@guybedford
Copy link
Owner

Agreed that now that we have full browser support conditional loading would be worth fleshing out more clearly.

This is something that would need to be designed as a core feature of the project because we have feature detection scripts which would still need to run. So effectively we would change the structure of es-module-shims to first include a simple feature detection script, that in turn would dynamically import the main polyfill only when needed.

There is still a tradeoff on that for a little while while browsers that don't support import maps would be penalized by that extra round trip. But as import maps gain in widespread usage this seems like a must-have to me.

@guybedford guybedford added the enhancement New feature or request label Apr 29, 2023
@bakura10
Copy link
Author

bakura10 commented May 4, 2023

For those trying the approach I have outlined (dynamically importing the module) this does not work all the time. I have timing issues, so I will wait for a built-in approach :D.

@bakura10
Copy link
Author

bakura10 commented Nov 8, 2023

For anyone interested in doing that, to make it work you actually need to append the element to the dom (and not try to dynamically importing it). This approach works to conditionally load the script:

<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>

@guybedford
Copy link
Owner

It seems like conditionally loading is working pretty well as Wordpress landed in https://github.com/WordPress/gutenberg/pull/57256/files using a HTMLScriptElement.supports('importmap') and document.write approach.

Perhaps the solution here is then just simply to document this as a first-class feature and continue supporting it as the primary approach going forward.

@luisherranz
Copy link

Perhaps the solution here is then just simply to document this as a first-class feature and continue supporting it as the primary approach going forward

That sounds good to me!

To be honest, I don't know why document.head.appendChild didn't work in our case. Maybe I did something wrong. I want to try again.

Or does it make sense that document.write works, but document.head.appendChild doesn't?

@guybedford
Copy link
Owner

I'm going to need some time to properly dig into the list of DOM attachments and implications for dynamic attachment. The current assumptions for the shim are very much that it's there first for performance reasons and is part of the loading lifecycle of the page (refiring dom events as necessary for the new loaded scripts). I suppose at least the document.write applies only in the legacy cases now so shouldn't apply for modern browsers at least, but agreed it would be preferable to figure out.

I'm going to track this issue and get back with further details when I find some time. In the mean time contributions are of course welcome too.

@luisherranz
Copy link

I suppose at least the document.write applies only in the legacy cases now so shouldn't apply for modern browsers at least

Yes. We don't mind releasing it in WordPress with document.write because it will only affect a small and shrinking percentage of users. So it's not critical at all. But I wanted to understand why it needs to use that method before committing to it.

The current assumptions for the shim are very much that it's there first for performance reasons and is part of the loading lifecycle of the page

There's one part that still doesn't make sense to me. Your recommendation is:

<script async src="es-module-shims.js"></script>

Async scripts can load at any time. They are not even guaranteed to load before the DOMContentLoaded event.

So, at least theoretically, there should be no difference between an async script and the document.head.appendChild method.

@guybedford
Copy link
Owner

Sorry for the delay here - if there is a well-tested PR that can support an appendChild attachment I'm open to landing that.

@luisherranz
Copy link

We finally went with document.write for WP 6.5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants