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

ESM Modules - Scripts 2.1 #5906

Closed
3 of 5 tasks
Maksims opened this issue Dec 19, 2023 · 14 comments
Closed
3 of 5 tasks

ESM Modules - Scripts 2.1 #5906

Maksims opened this issue Dec 19, 2023 · 14 comments

Comments

@Maksims
Copy link
Collaborator

Maksims commented Dec 19, 2023

As the community and developers using PlayCanvas express more need for a ESM Modules for Script Components, which is well discussed here: #4767.

This Issue is an alternative to #5764 in the way to implement ESM Modules with current ScriptType (Scripts 2.0).

Considerations for the feature:

  1. Reduce the migration/learning requirements with a new system.
  2. Provide same feature set as before (lazy-loading, hot-swap, concatenation, multi-scripts per file, attributes, events, etc).
  3. Developer should not require to import individual PlayCanvas engine features to be used, e.g. Vec3, Quat, etc. So use of global pc - is the same.
  4. Support tree shaking on exports.

Implementation:

  1. Improve ScriptType to better support of being extended by classes.
  2. Editor should support import's for modules when parsing attributes, as well as when loading in Launcher. Import Maps to be used.
  3. Building a project from Editor, should provide existing functionality of concatenation as well an additional features: e.g. tree shaking.
  4. Through Editor usage, developer should not worry about importing scripts, and they should be imported with support of multiple scripts and added to script registry automatically. Potentially adding a new asset type "Script Module" - would provide such distinction.

pc.ScriptType improvements:

  • Ability to class extend pc.ScriptType - this already works.
  • Support static class name.
  • If script name is not provided, use its class name as a script name.
  • Support static class attributes definition.
  • Add scripts from a module file to ScriptRegistry

Benefits of this approach:

  1. Same functionality as Scripts 2.0.
  2. Very small syntax adjustment for the developer.
  3. With a proper Editor integration of Import Maps, ability to import jsm files from vast number of npm packages and similar sources.

Concerns:

  1. Two very similar ways of achieving basically the same thing.
  2. Community has accumulated projects and a lot of info on code around the forums/github, that the newcomers have to be aware of the difference if decides to use Scripts 2.1 without prior experience with Scripts 2.0.

Proposed API:

Defining a ScriptType:

class TestScript extends pc.ScriptType {
    static name = 'testScript'; // optional

    static attributesData = [{ // optional
        name: 'speed',
        type: 'number',
        default: 42
    }];
    
    initialize() {
        console.log('script initialized');
    }

    update(dt) {
        this.entity.rotate(dt * this.speed, 0, 0);
    }
}

// ...

// attach a script to an entity, as before:
entity.script.create(TestScript);

Using jsm file uploaded to /scripts/utils.jsm in Editor:

import utils from '/scripts/utils.jsm';

// ...
@Maksims
Copy link
Collaborator Author

Maksims commented Dec 22, 2023

Based on the initial implementation, using static attributes would be somewhat difficult, as it conflicts with static attributes getter on the ScriptType. We can propose a different name for a property, e.g. attributesData.

@LeXXik
Copy link
Contributor

LeXXik commented Dec 22, 2023

One note about extending a ScriptType, which is marked as already supported. At the moment the support is only partial - it works fine in engine-only setups and at runtime, but Editor doesn't support extended scripts (e.g. they cannot be parsed).

@kungfooman
Copy link
Collaborator

Using jsm file uploaded to /scripts/utils.jsm in Editor:

Do you mean utils.mjs? https://nodejs.org/api/modules.html#the-mjs-extension

it works fine in engine-only setups and at runtime, but Editor doesn't support extended scripts (e.g. they cannot be parsed).

Yep, I would love to use proper ESM with relative import declarations in the editor, but the artificial file hierarchy (every asset receives some unknown ID) is also blocking it (I have no clue if that changed by now).

Parsing scripts for the editor could be done via Babel AST (for my sake they can also be in *.js files).

@LeXXik
Copy link
Contributor

LeXXik commented Dec 22, 2023

This reminds me. The ScriptType 2.0 cannot be used when trying to run engine in headless mode. The current script handler is using window. Perhaps 2.1 could be made so they do not depend on it.

@Maksims
Copy link
Collaborator Author

Maksims commented Dec 22, 2023

This reminds me. The ScriptType 2.0 cannot be used when trying to run engine in headless mode. The current script handler is using window. Perhaps 2.1 could be made so they do not depend on it.

We've used current script system in node.js.

One note about extending a ScriptType, which is marked as already supported. At the moment the support is only partial - it works fine in engine-only setups and at runtime, but Editor doesn't support extended scripts (e.g. they cannot be parsed).

Yes, indeed. We need to implement a good mjs solution for scripts in order to make it a good option for the Editor.
There two parts: parsing in Editor for attributes, and importing in Launcher / builds, which should also add it to script registry.
I will be experimenting and looking into it.

@LeXXik
Copy link
Contributor

LeXXik commented Dec 23, 2023

We've used current script system in node.js.

Right. I am referring to this document dependency:

_loadScript(url, callback) {
const head = document.head;
const element = document.createElement('script');
this._cache[url] = element;

I don't see how it can be done without some polyfill or shimming. Since this is a new design, could this dependency be removed as well?

@Maksims
Copy link
Collaborator Author

Maksims commented Dec 23, 2023

We've used current script system in node.js.

Right. I am referring to this document dependency:

_loadScript(url, callback) {
const head = document.head;
const element = document.createElement('script');
this._cache[url] = element;

I don't see how it can be done without some polyfill or shimming. Since this is a new design, could this dependency be removed as well?

Oh this, that will have to change, depending on context.
E.g. in node.js, we load scripts differently: https://github.com/meta-space-org/playnetwork/blob/main/src/server/libs/scripts.js#L229-L255

This is also complex for modules, as currently, dynamic import does not work in modern browsers.

@marklundin
Copy link
Member

Modules would need to be treated differently to other scripts and loaded as type='module'. Additionally the current concatenation method which just appends appends files together wouldn't work as it changes module scope. It needs a bundle system. Also, it's probably fair to assume that if a user elects to use es modules, then dynamic imports are available right?

@marklundin
Copy link
Member

marklundin commented Jan 17, 2024

Just thinking about this @Maksims, we can't really guarantee the loading order of ES Modules scripts. Consider the following ScriptType files:

// User specified loading order
./script-a.mjs
./script-b.mjs
./script-c.mjs

// script-a
import './script-c'
console.log('a')

// script-b
console.log('b')

// script-c
console.log('c')

// output of loading
c, a, b

Example

This may be ok though, as we just exclude ES Module Scripts from the loading order API. In addition any es module should probably be excluded from the loadingOrder, as it'll just be imported from a script directly. So we could just keep the loading order for existing classic ES5 scripts.

@Maksims
Copy link
Collaborator Author

Maksims commented Jan 17, 2024

This may be ok though, as we just exclude ES Module Scripts from the loading order API. In addition any es module should probably be excluded from the loadingOrder, as it'll just be imported from a script directly. So we could just keep the loading order for existing classic ES5 scripts.

I believe you are right. ES Modules cannot be controlled, and their order of loading should be based on how they are imported in various scripts. Also, because they are executed in isolated namespace, their local variables are not shareable, and for referencing some object from another module, it must be imported - so it will guarantee the loading order.

So indeed, they should be excluded from the loading order API, but the leaves of the import-tree should be loaded still.
In context of engine-only, it is up to a developer to deal with it. In Editor case, I think it should be well communicated, and decided that either it loads before or after normal scripts. Worth investigating.

@marklundin
Copy link
Member

marklundin commented Jan 18, 2024

One thing that could become more problematic is that attributes with getters/setters would not work using ScriptType

class Rotator extends ScriptType {
    _offset = 0;
    set offset (value) {
      _offset = value % 360;
    }
}

// Overrides the offset
Rotator.add('offset', { ... })

script.Rotator.offset = 370// By passes the setter

This is not inherently specific to ES6 classes, but it feels like a much more common scenario. Not sure what the options are here.

@Maksims
Copy link
Collaborator Author

Maksims commented Jan 18, 2024

One thing that could become more problematic is that attributes with getters/setters would not work using ScriptType

class Rotator extends ScriptType {
    __offset = 0;
    set offset (value) {
      _offset = value % 360;
    }
}

// Overrides the offset
Rotator.add('offset', { ... })

script.Rotator.offset = 370// By passes the setter

This is not inherently specific to ES6 classes, but it feels like a much more common scenario. Not sure what the options are here.

I thought about this, and there is an option: when accessors are created based on attributes, we can check if such property is already defined on the instance, and so we can skip overriding. It will have a logical impact: if developer defines own properties / accessors, then events and copying of provided values will not work. But I believe this should be a separate PR.

@marklundin
Copy link
Member

In terms of just engine support as per #5963, we can probably close this out now right @Maksims?

@Maksims
Copy link
Collaborator Author

Maksims commented Feb 29, 2024

I think yes.
One thing to test with another PR, is how it works with script-name, when async adding to script registry, to trigger either initialize or swap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants