engine icon indicating copy to clipboard operation
engine copied to clipboard

ESM Modules - Scripts 2.1

Open Maksims opened this issue 1 year ago • 12 comments

As the community and developers using PlayCanvas express more need for a ESM Modules for Script Components, which is well discussed here: https://github.com/playcanvas/engine/issues/4767.

This Issue is an alternative to https://github.com/playcanvas/engine/pull/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:

  • [x] Ability to class extend pc.ScriptType - this already works.
  • [x] Support static class name.
  • [x] 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 avatar Dec 19 '23 16:12 Maksims

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.

Maksims avatar Dec 22 '23 15:12 Maksims

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

LeXXik avatar Dec 22 '23 15:12 LeXXik

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

kungfooman avatar Dec 22 '23 16:12 kungfooman

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.

LeXXik avatar Dec 22 '23 19:12 LeXXik

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.

Maksims avatar Dec 22 '23 20:12 Maksims

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

Right. I am referring to this document dependency: https://github.com/playcanvas/engine/blob/dfc4512e89d9eee155f34087f1a4d54ffc6397bf/src/framework/handlers/script.js#L99-L102

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?

LeXXik avatar Dec 23 '23 14:12 LeXXik

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

Right. I am referring to this document dependency:

https://github.com/playcanvas/engine/blob/dfc4512e89d9eee155f34087f1a4d54ffc6397bf/src/framework/handlers/script.js#L99-L102

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.

Maksims avatar Dec 23 '23 15:12 Maksims

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 avatar Jan 04 '24 11:01 marklundin

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.

marklundin avatar Jan 17 '24 11:01 marklundin

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.

Maksims avatar Jan 17 '24 14:01 Maksims

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.

marklundin avatar Jan 18 '24 09:01 marklundin

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.

Maksims avatar Jan 18 '24 10:01 Maksims

In terms of just engine support as per https://github.com/playcanvas/engine/pull/5963, we can probably close this out now right @Maksims?

marklundin avatar Feb 29 '24 11:02 marklundin

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.

Maksims avatar Feb 29 '24 14:02 Maksims