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.

Sign up for GitHub

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

Jump to bottom

Add a check on app block for preventing importmap #647

Open
bakura10 opened this issue Dec 4, 2024 · 12 comments
Open

Add a check on app block for preventing importmap #647

bakura10 opened this issue Dec 4, 2024 · 12 comments
Assignees
@AribaRajput
Labels
area:theme-check enhancement New feature or request good first issue Good for newcomers

Comments

@bakura10
Copy link

bakura10 commented Dec 4, 2024

Hello!

In our recent themes I am using import map. Those are awesome.

However, while this has not happened yet, I’m pretty sure some app dev will create an app block in the head that create an import map.

Because only one import map can exist per document, if an app create an import map this will break the theme completely. An app dev might not realize it if they are testing on a theme (such as Dawn) that is not using import map.

As a prevention, theme checker should emit an error if an app block contains a script whose type is importmap (https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script/type/importmap).

Thanks
@charlespwd
Copy link
Contributor

Because only one import map can exist per document.

@yoavweiss I vaguely recall having us having a discussion about this. Did this change after all?

@yoavweiss
Copy link
Member

Multiple import maps are shipping in Chromium 133, which will reach Chrome stable in early February.

At the same time, I haven't started the WebKit implementation just yet.

@yoavweiss
Copy link
Member

Also note that if you're using an import map in browsers that don't support multiple import maps, even a module script loaded before your import map would break the theme.

@bakura10
Copy link
Author

bakura10 commented Dec 4, 2024

I didn't know about multiple import maps, that's so cool! I still think we should have at least a warning in app blocks to discourage app author to do it until multiple import maps are widely supported.

@yoavweiss
Copy link
Member

I didn't know about multiple import maps, that's so cool! I still think we should have at least a warning in app blocks to discourage app author to do it until multiple import maps are widely supported.

I tend to agree we need to avoid those scenarios until coverage is better, or alternatively load a polyfill to smooth the gaps over in UAs that don't support multiple import maps.

@bakura10
Copy link
Author

bakura10 commented Dec 4, 2024

I actually was not even aware of the limitation regarding a module being loaded before the importmap. That's good to know! And even easier for app dev to not realize that this may break based on the theme (Dawn does not use module anywhere, while all our newer themes do). We should play it safe here!

@charlespwd
Copy link
Contributor

charlespwd commented Dec 4, 2024 •
edited
Loading

TBF I'm not sure the building blocks are there. IIRC the API for app blocks & assets is fairly limited.

Apps can load assets using either the JavaScript and stylesheet schema attributes or from the asset_url and asset_img_url Liquid URL filters.

From what I understand and remember, this means that the platform controls how the scripts are injected (and IIRC at the bottom of <head>) and there's no way to target the head otherwise.

Same for app embeds. You'd target the bottom of <head> or the bottom of <body>.

The only surface I could think of is an app embed that targets compliance_head. Is this the situation you're worried about?

EDIT: I guess a compliance_head could have a script type=module, but I guess that defer-ing would go against the goal they are after? From what I understand that's reserved for SEO tags and cookie banners. (Trying to establish what the rule for the check should be if we made one).

@bakura10
Copy link
Author

bakura10 commented Dec 4, 2024

Yes, the problem are app embed in the . An app embed in the head could do this:

<script type="importmap">
{
  SOME STUFF
}
</script>

<script type="module" src="{{ 'something.js' | asset_url }}" />

The module something.js would be able to load modules defined in the importmap in Dawn, for instance (as Dawn does not have module and does not use importmap, it would comply with the rules of importmap).

If the same app embed would be on Prestige or Impact, it would break the theme. :D

@charlespwd
Copy link
Contributor

charlespwd commented Dec 4, 2024 •
edited
Loading

Yeah that's fair and I think a reasonable ask.

Check requirements

  • This is an Error severity check
  • Everywhere in theme app extensions
    • Report offence when using <script type="importmap">
    • Offence message: Until browsers permit multiple importmap entries, only themes can have an importmap
  • For app embeds targeting compliance_head
    • Report offence when using <script type="module"> (may show up before theme importmap)
    • Offence message: Compliance App embeds cannot use <script type="module">
      • We can have a link to the docs that explain why

@charlespwd charlespwd added enhancement New feature or request good first issue Good for newcomers area:theme-check labels Dec 4, 2024
@AribaRajput AribaRajput self-assigned this Dec 17, 2024
@AribaRajput AribaRajput mentioned this issue Jan 2, 2025
Open
8 tasks
@yoavweiss
Copy link
Member

Multiple import maps are shipping soon in Chromium and in a future Safari.

But given our legacy browser support requirements, we'd probably also need a polyfill that catches the errors multiple import maps cause in (soon-to-be) legacy browsers and recovers from them (e.g. by loading them again using system.js).

@yoavweiss
Copy link
Member

@guybedford pointed out to me that es-module-shims is the right polyfill for this, and that it recently got multiple import map support.

@bakura10
Copy link
Author

bakura10 commented Jan 6, 2025

I’ve been using this polyfill for nearly 2 years on our themes using importmap, never had a problem, it works really well!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@AribaRajput AribaRajput

Labels
area:theme-check enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@yoavweiss @bakura10 @charlespwd @AribaRajput