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

[5.0] ESM importmap support #40714

Merged
merged 13 commits into from
Jun 24, 2023
Merged

[5.0] ESM importmap support #40714

merged 13 commits into from
Jun 24, 2023

Conversation

Fedik
Copy link
Member

@Fedik Fedik commented Jun 4, 2023

Summary of Changes

This PR add support for <script type="importmap"> in to WebAsset manager.

The browser support is limited to only 2023 year version, but that should not stop us.
As for now, I have added a polyfill. helper method to work around browser support for "top level" imports.

What changed:

joomla.asset.json added next properties for "script" asset:
importmap boolean, whether the element should be added to "importmap";
importmapOnly boolean, whether the element should be ONLY in "importmap", and do not render it as <script>;
importmapName string, optional, custom module name, example when asset name foo, and module name is @foo;

Fallback for orlder browser

Uses es-module-shims polyfill.

Because of browser support I have added esm-map asset, wich provide joomlaESMap() function to resolve module to URL when "importmap" not supported.

Untill browser support will get better we should use the import as following:

PS: I know about polyfill, but I do not like how that works.

Testing Instructions

Apply path, run npm install.
Create 2 js file:
media/system/js/test-module.js with content:

const bla = {
  blabla: 1
}
export { bla }

media/system/js/test.js with content:

import { bla as bla1 } from '@test-module';
import { bla as bla2 } from 'system/test-module.js';

console.log(bla1, bla2);

Add them to media/system/joomla.asset.json:

{
      "name": "test-module",
      "type": "script",
      "uri": "system/test-module.js",
      "importmap": true,
      "importmapName": "@test-module",
},
{
      "name": "test-module2",
      "type": "script",
      "uri": "media/system/js/",
      "importmap": true,
      "importmapName": "system/",
},
{
      "name": "test",
      "type": "script",
      "uri": "system/test.js",
      "attributes": {
        "type": "module"
      },
      "dependencies": [
        "test-module", "test-module2"
      ]
}

In index.php of the template, enable this "testing asset" with $wa->useScript('test');

Open the site in browser, and look in browser console.
You should get console log of Object from "test-module".

Link

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-5.0-dev labels Jun 4, 2023
@Fedik Fedik added the Feature label Jun 4, 2023
@dgrammatiko
Copy link
Contributor

Hats off Fedir! This is the most important feature for j5 for the modern js…

@Fedik
Copy link
Member Author

Fedik commented Jun 5, 2023

I have added es-module-shims polyfill. but it 30Kb minified :)

@dgrammatiko
Copy link
Contributor

@Fedik Is it possible to use $wam->disable('es-module-shims'); as we were discussing with Harald? LGTM

@Fedik
Copy link
Member Author

Fedik commented Jun 5, 2023

Yes, can be done onBeforeCompileHead event.

@HLeithner
Copy link
Member

Still not sure why we need importmaponly parameter, maybe I have not enough understanding that someone needs both.

@Fedik
Copy link
Member Author

Fedik commented Jun 21, 2023

Not allowing both will be an unneeded limitation.
There may be a reasons why need both, starting from simple "preload" to some fancy logic, when example the code init the script and doing export at same time (not like it is a good practice but it is a web).

That why I think that we need a way to have it both, without duplicating the asset declaration.

It can be other way around, when importmap will add the script only to the map.
And some extra option renderThisScript which also render it, or can check if "attribute type:module" is present then also render (but this can be a bit cryptic for developers).
I am fine with any.

@Fedik
Copy link
Member Author

Fedik commented Jun 21, 2023

Hmhm, or maybe I will remove it. Duplicated asset with "cross dependency" also can work to allow both.
I will update PR sometime

@Fedik
Copy link
Member Author

Fedik commented Jun 22, 2023

I have removed importmapOnly.
And updated description, with example of use of a directory in the importmap.

@HLeithner
Copy link
Member

ok I would say we have a MVP here and can merge it do we have a test case for it which where we can use it in core? I think codemirror maybe too complicated as simple test suite?

@dgrammatiko @Fedik @laoneo

@dgrammatiko
Copy link
Contributor

@HLeithner #41018 is a more realistic example

@Fedik
Copy link
Member Author

Fedik commented Jun 23, 2023

Also probably can be Bootstrap js.

@HLeithner
Copy link
Member

Also probably can be Bootstrap js.

Would be great, not sure if this is possible in a b/c way?

@dgrammatiko
Copy link
Contributor

Would be great, not sure if this is possible in a b/c way?

it’s backward compatible. The only problem with switching to import maps is the extra polyfill cost and the fact that to get the es5 scripts there’s a need for some changes in the build tools. Assuming that the es5 will be dropped in v5 the only consideration is the polyfill cost.

btw is there a plan to merge the pr for the drop of es5?

@dgrammatiko
Copy link
Contributor

Also probably can be Bootstrap js.

All the .es6.js that have an import can be switched to import maps: guided tours, keyboard short cuts, choices custom element, form validator, etc

Also it’s an easy task, change the import, add the module to be imported to the dependencies and add an entry to the assets json. Job done!

@HLeithner
Copy link
Member

Would be great, not sure if this is possible in a b/c way?

it’s backward compatible. The only problem with switching to import maps is the extra polyfill cost and the fact that to get the es5 scripts there’s a need for some changes in the build tools. Assuming that the es5 will be dropped in v5 the only consideration is the polyfill cost.

btw is there a plan to merge the pr for the drop of es5?

yes is planned, wasn't able to test it personally yet

@HLeithner HLeithner merged commit de1a338 into joomla:5.0-dev Jun 24, 2023
@HLeithner
Copy link
Member

Thanks, I tested it and merged it for now and hope for more tests in alpha2, also I think we should remove the default polyfill

@Fedik
Copy link
Member Author

Fedik commented Jun 24, 2023

PR for manual joomla/Manual#134
PR for Schema joomla/schemas#17

@dgrammatiko
Copy link
Contributor

@Fedik the nonce attribute for the CSP is respected as far as I understand the renderAttributes function. Am I right?

Also shouldn't we disallow some other attributes there, ie async, defer, noscript, etc?

@Fedik
Copy link
Member Author

Fedik commented Jun 24, 2023

Yeap CSP should be there for importmap inline script.

Also shouldn't we disallow some other attributes there, ie async, defer, noscript, etc?

For wich case? For script with importmap: true they will be ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature NPM Resource Changed This Pull Request can't be tested by Patchtester
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants