ESM ScriptType #5963

marklundin · 2024-01-22T13:59:33Z

This PR adds support for es modules using the existing ScriptType system. It conditionally loads scripts as a module depending on the file name (ending in.mjs). This can be used in the editor as-is although there is work necessary in the editor to parse Scripts as modules.

Issues

Scripts cannot define attributes as getter/setters. Scripts 2.1 (Class support) #5910 (comment)
Script class names can be mangled during a build, which would registerScript Scripts 2.1 (Class support) #5910 (comment)

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

slimbuck · 2024-01-22T14:05:13Z

src/framework/handlers/script.js


-        head.appendChild(element);
+        const path = url.split('?')[0];
+        const isEsmScript = path.endsWith('.esm.js');


wondering why not .mjs?

Open to suggestion, however one thing the V8 team and Mozilla pointed out is that '.js' will get served with the correct Content-Type headers that would already exist on the server. '.mjs', '.jsm' etc may not have those configured and would need to be manually setup. Okay for us, but maybe difficult if users don't have access to their server configuration. (Source MDN)[https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Modules#aside_%E2%80%94_.mjs_versus_.js]

Although build systems, today do support mjs pretty well, e.g. Babel.
We do have page that does require setting up server already: https://developer.playcanvas.com/en/user-manual/publishing/web/self-hosting-for-beginners/#content-types
It worth adding there configs too.

That's true. I'm not particularly wedded to either. Currently the editor doesn't let you change the file suffix, so that would need to be changed, and I'm not sure what it really buys you. The nice thing about keeping it '.js' is that the user can just change directly as-is. Also one other thing, there's gonna be a distinction between ScriptType modules, and just plain vanilla modules (config/utils/etc). Arguably these shouldn't only work if they're '.mjs'. Also potentially, if we enforced the ESM ScriptType at the file name level then we can guard against it being added as a script in the editor, rather than having to load it and check through exports.

Maksims · 2024-01-22T14:15:56Z

src/framework/handlers/script.js

+
+            const baseUrl = platform.browser ? window.location.origin : '';
+            const importUrl = new URL(url, baseUrl);
+            importUrl.searchParams.append('cacheBust', new Date().valueOf().toString());


As script is an asset, we should use asset.file.hash here to ensure that cache can be used reliably.

Ahh the file.hash changes on asset save! Of course it does :D

mvaligursky · 2024-01-24T14:54:11Z

Personally, I'd like to see an example of this, perhaps with limitations in comments do demonstrate what works and what does not. It does not need to be an engine example (but could be), but at least one in the PR description, so that we know what this enables / what needs to be avoided, for this to work in engine only / editor environment.

marklundin · 2024-01-24T15:34:21Z

@mvaligursky There isn't any changes in the engine use, it's only where files get loaded in the Script Handler. Here's a Editor example, which will work if you run the launcher or edit code. Although don't parse the script in the editor as this needs an editor update

https://playcanvas.com/editor/scene/1927088

mvaligursky · 2024-01-24T15:44:41Z

Great example. So how do we let people know they can use it?

LeXXik · 2024-01-24T15:49:24Z

The example doesn't work for me or do I need to run it against this PR?

mvaligursky · 2024-01-24T15:50:10Z

The example doesn't work for me or do I need to run it against this PR?

Yes you would need that.

marklundin · 2024-01-24T15:52:54Z

I have an editor fix that parses attributes which we'd need before recommending to users.

Also we'd need to get documentation in place

mvaligursky · 2024-01-24T16:22:54Z

src/framework/handlers/script.js

+        const importUrl = new URL(url, baseUrl);
+
+        // @ts-ignore
+        import(importUrl.toString()).then((module) => {


So this uses import, which based on https://caniuse.com/?search=import is supported by Chrome from 2017, Safari 2018, Firefox 2019 - I wonder how we could handle the compatibility issues here.

What happens on browsers using old script style .. are they going to get exception when this code gets parsed? Or is there some babel polyfill for this?

There has to be an assumption that if a user chooses to use ES modules, then there is a neccessary baseline level of support required. ES6 is around 97.2% which is only 0.7% less than our legacy build.

If someone uses the old script mechanism, the ESM path won't get triggered so it will just work as is.

slimbuck · 2024-01-24T16:40:25Z

src/framework/handlers/script.js

+    _loadModule(url, callback) {
+
+        // if we're in the browser, we need to use the full URL
+        const baseUrl = platform.browser ? window.location.origin : import.meta.url;


Can an es5 version of the engine load module scripts? If so then import.meta.url would fail, right?

(Even though that'll only ever be used in node).

The target is marginally higher than ES5 now, but babel should transpile this according to the preset.env. See here

That sounds good, just double check it does and we're not missing some settings please.

LeXXik · 2024-01-24T17:04:50Z

Will the script that was extended from ScriptType be parsed by Editor as well?

marklundin · 2024-01-24T17:45:50Z

Will the script that was extended from ScriptType be parsed by Editor as well?

How do you mean @LeXXik? Do you mean subclassing something that extends ScriptType?

class CustomBase extends ScriptType { static attributeDefinitions: { ... } }
class MyClass extends CustomBase {}

This should work and pick up the correct attributes if you attach MyClass to a Script Component, ie, it will 'inherit' the CustomBase attributes. Although, for the moment you can't import a relative module from the asset registry. Not until we address the pathing issues.

Maksims · 2024-01-24T17:53:09Z

Will the script that was extended from ScriptType be parsed by Editor as well?

I think you just mean if current Editor workflow will work pretty much the same with a ESM script? It will.

class Test extends pc.ScriptType { }
pc.registerScript(Test, 'test');

This already works in Editor.

With ESM script no need to call pc.registerScript (you still should be able to though), instead you would export, like so:

export Test;

And asset loader will automatically add it to the script registry.

marklundin · 2024-01-24T18:00:43Z

Yep, all exports from a module will be tested to see if they are a valid ScriptType, so the following are both valid Scripts

class Test extends ScriptType {};
class OtherTest extends ScriptType {};

export Test;
export default OtherTest;

LeXXik · 2024-01-24T18:00:57Z

Right, I was referring to the child class, as in

class A extends pc.ScriptType {}
class B extends A {} // can't parse B in Editor at the moment

Maksims · 2024-01-25T06:58:13Z

Right, I was referring to the child class, as in

class A extends pc.ScriptType {}
class B extends A {} // can't parse B in Editor at the moment

That would be the Editor. Technically it could already be improved to load other scripts. With ESM Modules and import maps - this becomes easier. You would need to do this:

/scripts/scriptA.js

class A extends pc.ScriptType {}
export A;

/scripts/scriptB.js

import A from '/scripts/scriptA.js';
class B extends A {}
export B;

Maksims

LGTM!

slimbuck

🎉

marklundin · 2024-01-25T16:04:24Z

Quietly pretty excited about this one!

marklundin added 4 commits January 22, 2024 12:35

Add support for importing ES modules in Script Handler

60cda4f

Add platform check

b60ca97

Fixed import path

25dcfa0

Better check if script is subclass of ScriptType

2ee13cb

marklundin changed the title ~~Esm loading~~ ESM ScriptType loading Jan 22, 2024

marklundin requested a review from mvaligursky January 22, 2024 14:02

Merge branch 'main' into esm-loading

f9013b7

slimbuck reviewed Jan 22, 2024

View reviewed changes

Maksims reviewed Jan 22, 2024

View reviewed changes

marklundin and others added 2 commits January 24, 2024 14:44

Using file hash instead of time for cache busting

f5bdda0

Merge branch 'main' into esm-loading

91aae0b

marklundin requested review from slimbuck and willeastcott January 24, 2024 14:46

mvaligursky requested a review from Maksims January 24, 2024 14:50

Fixed linting + tests

2782805

linting fix

de9e27e

mvaligursky reviewed Jan 24, 2024

View reviewed changes

slimbuck reviewed Jan 24, 2024

View reviewed changes

Merge branch 'main' into esm-loading

d0623e4

Changed ESM requirement to having '.mjs' suffix

3931ddd

Maksims approved these changes Jan 25, 2024

View reviewed changes

willeastcott approved these changes Jan 25, 2024

View reviewed changes

slimbuck approved these changes Jan 25, 2024

View reviewed changes

marklundin merged commit c80ab34 into main Jan 25, 2024
7 checks passed

mvaligursky mentioned this pull request Jan 29, 2024

UMD broken in older browsers #6011

Closed

marklundin deleted the esm-loading branch February 14, 2024 12:46

This was referenced Feb 29, 2024

ESM Modules - Scripts 2.1 #5906

Closed

Allow imported scripts to load as modules playcanvas/editor#1054

Closed

marklundin self-assigned this Feb 29, 2024

marklundin added the area: scripts label Feb 29, 2024

marklundin changed the title ~~ESM ScriptType loading~~ ESM ScriptType Mar 5, 2024

marklundin mentioned this pull request Mar 11, 2024

Editor is unable to parse extended scripts playcanvas/editor#188

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESM ScriptType #5963

ESM ScriptType #5963

marklundin commented Jan 22, 2024 •

edited by willeastcott

Loading

slimbuck Jan 22, 2024

marklundin Jan 22, 2024 •

edited

Loading

Maksims Jan 22, 2024

marklundin Jan 22, 2024 •

edited

Loading

Maksims Jan 22, 2024 •

edited by willeastcott

Loading

marklundin Jan 22, 2024 •

edited

Loading

mvaligursky commented Jan 24, 2024

marklundin commented Jan 24, 2024

mvaligursky commented Jan 24, 2024

LeXXik commented Jan 24, 2024

mvaligursky commented Jan 24, 2024

marklundin commented Jan 24, 2024 •

edited

Loading

mvaligursky Jan 24, 2024

mvaligursky Jan 24, 2024

marklundin Jan 24, 2024

slimbuck Jan 24, 2024

slimbuck Jan 24, 2024

marklundin Jan 24, 2024

mvaligursky Jan 24, 2024

LeXXik commented Jan 24, 2024

marklundin commented Jan 24, 2024 •

edited

Loading

Maksims commented Jan 24, 2024

marklundin commented Jan 24, 2024

LeXXik commented Jan 24, 2024

Maksims commented Jan 25, 2024

Maksims left a comment

slimbuck left a comment

marklundin commented Jan 25, 2024

ESM ScriptType #5963

ESM ScriptType #5963

Conversation

marklundin commented Jan 22, 2024 • edited by willeastcott Loading

Issues

Choose a reason for hiding this comment

marklundin Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marklundin Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

Maksims Jan 22, 2024 • edited by willeastcott Loading

Choose a reason for hiding this comment

marklundin Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

mvaligursky commented Jan 24, 2024

marklundin commented Jan 24, 2024

mvaligursky commented Jan 24, 2024

LeXXik commented Jan 24, 2024

mvaligursky commented Jan 24, 2024

marklundin commented Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeXXik commented Jan 24, 2024

marklundin commented Jan 24, 2024 • edited Loading

Maksims commented Jan 24, 2024

marklundin commented Jan 24, 2024

LeXXik commented Jan 24, 2024

Maksims commented Jan 25, 2024

Maksims left a comment

Choose a reason for hiding this comment

slimbuck left a comment

Choose a reason for hiding this comment

marklundin commented Jan 25, 2024

marklundin commented Jan 22, 2024 •

edited by willeastcott

Loading

marklundin Jan 22, 2024 •

edited

Loading

marklundin Jan 22, 2024 •

edited

Loading

Maksims Jan 22, 2024 •

edited by willeastcott

Loading

marklundin Jan 22, 2024 •

edited

Loading

marklundin commented Jan 24, 2024 •

edited

Loading

marklundin commented Jan 24, 2024 •

edited

Loading