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

ES Module Scripts #4767

Open
willeastcott opened this issue Oct 19, 2022 · 37 comments
Open

ES Module Scripts #4767

willeastcott opened this issue Oct 19, 2022 · 37 comments

Comments

@willeastcott
Copy link
Contributor

willeastcott commented Oct 19, 2022

The current PlayCanvas script format was introduced in July 2016. PlayCanvas scripts are snippets of JavaScript that are (asynchronously) loaded and then, once loaded, immediately executed in global application scope.

The current PlayCanvas script format has a number of problems:

  1. The current default script template is still generated based on the ES5 feature set of JavaScript. This is looking increasingly antiquated in a world where ES Modules have become dominant. JavaScript has moved on considerably over the last 10 or so years.
  2. Developers want to leverage tree shaking to generate an application bundle that only includes the code necessary to execute the application. The current script format is not compatible with tree shaking because it does not specify the specific symbols on which the code relies. ESM does this with the import statement.
  3. Developers want to be able to run a build system (like Rollup, Webpack etc). This would allow them to run transpilers (e.g. Babel), minifiers (e.g. Terser) and so on. These build tools (or bundlers) tend to take ES Modules as input. Our current script format does not play nice with these bundlers.
  4. While you can run PlayCanvas in Node if you use the ES5 build of the engine, the current script system cannot be used with the module build (or if you run the source directly in Node, as with our engine unit tests). When the scripts are loaded (by doing a dynamic import call), the pc namespace doesn't exist. The script would need to import what it needs.
  5. Intellisense in VS Code (or the Code Editor's Monaco) does not work too well with ScriptType-based scripts. For example, attributes don't resolve correctly. An ES Module containing a class might be a better approach.

So I propose that we investigate the development of a new script format that:

  • Is based on ES Module format.
  • Is based on ES6 class syntax.
  • Supports dynamic loading (say when running the Launch tab from the PlayCanvas Editor).
  • Supports easy-to-use hot reloading.
  • Supports being built into a bundle (by the likes of Rollup etc).
  • Supports Intellisense well in VS Code/Monaco.
  • Can be run in Node when running the engine as a module or from source modules (this is critical for unit testing in Node, say).
  • Is compatible with TypeScript.

I welcome comments from the community on this proposal.

@Maksims
Copy link
Contributor

Maksims commented Oct 20, 2022

Would be great to see an example of most minimal script, as well as a script that uses attributes, and some few other features in the code.

@nikitaosyak
Copy link

nikitaosyak commented Oct 21, 2022

Hi Will!

I'm thrilled to see open discussion about it. Let me share how we do stuff, working around pc quirks.

We use webpack bundling and imports inside webstorm IDE, which works very good with jsdoc, so we have full intellisense as if working with typescript (well, almost)

We also use inheritance in rare cases, also with attributes, and here is how:

// imports work fine with webpack
import UtilShader from "../UtilShader.js"; 

//
// for attributes to make it into intellisense, we have to annotate 
// them with @property keyword. that's a chore, but works fine. 
// Would like to see it in some better form
/**
 * @class {PostEffectBase}
 * @property {boolean} skip
 */
//
// in webstorm, all playcanvas classes are available from 
// pc.* "namespace", so we can intellisense any type, 
// including entities, colors, curves, etc
export default class PostEffectBase extends pc.ScriptType {

    //
    // this is a string for script registry, for better 
    // refactoring purposes, also we can have generic 
    // getComponent with ClassName.pcName using this 
    static pcName = 'postEffectBase' 

    //
    // here we assign attributes in such a way that it can
    // be both added to descendant of current class, otherwise
    // attributes would not be able to propagate
    /**
     * @param {pc.ScriptType} v
     */
    static registerAttributes(v) {
        v.attributes.add('skip', {type : 'boolean', default : false})
    }

    //
    // we can have some class members, protected or private,
    // based on _ or __ notation, but in most cases I don't
    // care for access modifiers :)
    /** @type {pc.RenderTarget} */
    targetInput = null
    
    //
    // ofc we will have script lifecycle methods, which works fine
    initialize() {
        // do something here
    }
    
    //
    // we can have methods with typed arguments
    // and return values, which works fine
    /**
     * @param {pc.RenderTarget} v
     */
    setRenderTarget(v) {
        this.targetInput = v
    }
}

//
// finally, register resulting scripts in playcanvas registry
pc.registerScript(PostEffectBase, PostEffectBase.pcName)

// register attributes for this specific class,
// descendant will use both base class register attributes, and
// own register attributes
PostEffectBase.registerAttributes(PostEffectBase)```

@nikitaosyak
Copy link

nikitaosyak commented Oct 21, 2022

we also don't use script hotswap, since it's a chore to write around, since I have to lay off my current state, save it and return to new script, but we do use swap method to live-reload whole app after any change, since we're using file watchers and rest API to load bundle into playcanvas editor. for that, we have reload on swap script:

const ReloadByswap = pc.createScript('reloadByswap');

ReloadByswap.prototype.swap = function(old) {
    window.location.reload();
};

export default ReloadByswap```

since it's included in bundle which uploads to editor, every time we change stuff, dev build is being reloaded :)

@nikitaosyak
Copy link

regarding vs code or monaco I don't really know why use them. I would better look at IDE headliner, like intellij, since they are just better IDE's, and they have better intellisense. but that's like my humble opinion.

@yaustar
Copy link
Contributor

yaustar commented Oct 21, 2022

Be able to have the script attributes auto complete with type in any code editor. The way they are currently, doesn't allow code editors to infer the property or the type

@yaustar
Copy link
Contributor

yaustar commented Oct 21, 2022

There is another issue with the Editor that we would need to consider, there's no real folder structure. Each asset is in a folder with the name as the resource id. This would make importing other scripts/files nearly impossible unless we change this somehow or have a build step for each launch

@willeastcott
Copy link
Contributor Author

@yaustar That's a really good point. Obviously, modules need to specify import paths. So I'm not sure how we'd support that...

@yaustar
Copy link
Contributor

yaustar commented Oct 21, 2022

Did the old Scripts V1 workflow and the 'scripts folder' specialised in that respect where they were stored differently? Not entirely sure if that's something we want to go back to.

@Maksims
Copy link
Contributor

Maksims commented Oct 22, 2022

For imports, worth looking at import maps.

If writing a script would require to import all dependencies instead of current approach, this will have worse learning and usability curve.

@marklundin
Copy link
Member

marklundin commented Nov 3, 2022

This is really exciting to see. I've spent a bit of time thinking about this so here's a braindump...

Ideas for a Module Type

Just as a thought starter, I've always thought Decorators are a good candidate for this kind of thing. Similar to Unity Attributes Consider the following:

import { attribute, inspectable } from 'playcanvas'

@inspectable("My Awesome Class", "This description is surfaced to the editor")
class MyClass {

  @attribute("This attribute title is exposed to the editor")
  nSomeValue= "This is a default value"

  initialize(){}
  destroy(){}
  update(){}
}

Of course this is still a Stage 3 Proposal so without TypeScript or other transpiler the same code above would reduce to a slightly more clunky:

import { attribute, inspectable } from 'playcanvas'

class MyClass {

  nSomeValue= 'This is a default value'

  initialize(){}
  destroy(){}
  update(){}
}

inspectable("My Awesome Class", "This description is surfaced to the editor")(MyClass)
attribute('This title is exposed to the editor')(MyClass.prototype.nSomeValue)

The decorators are composable so you can includes things like constraints. The nice thing about this is it's self documenting.

Thoughts on ES Modules

Module resolution Problem
Module paths should resolve in a way synonymous to the asset registry. Unfortunately Import Maps won't work here as it apparently only allows you to map an import to a url so that:

import moment from 'moment'

becomes

import moment from 'https://unpkg.com/moment'

It doesn't allows you to modify the path resolution algorithm so that a relative path like '../../path/to/module' resolves to 'launch.playcanvas.com/api/assets/files/asset/script.js?id=123&branchId=231'

How to solve this:

  1. HTTP Server. Update the backend to serve scripts in the same virtual filesystem as the asset registry. This may be the most ideal scenario as it removes the need to do any import re-mappings, however there's likely legitimate reasons why this isn't already the case.

  2. Pre-Processor. A build step that resolves and rewrites paths ahead-of-time or at run time. Doing this at runtime though is only likely to increase app startup time as you'd need to traverse the dependency tree. So there is an alternative argument for doing this ahead-of-time as a pre-processing step using a well supported transpiler. This also opens the door for things like TypeScript and JSX and other goodness.

Transpilers
Bundling may not be strictly necessary when using ES modules and HTTP2, however a pre-processing step has other benefits. For example you can also support things like TypeScript or JSX, inject environment variables, strip out non production code etc. There are obvious complexities to this and it's likely to be outside the scope of the current issue, but in our experience developing the PC Package Manager it's still very much desirable and doable at edit time.

Potential issues with ES Modules

  • Module scope
    Some scripts in existing projects may depend upon implicitly defined global variables in other scripts. Scripts all share the same global scope however modules have their own scope so adding type='module' to existing project scripts may break things. Users may have to explicitly opt-in to modules

  • Error Handling
    In the PC launcher there is a global error handler that catches any error and provides a useful mechanic to link back to the source code in the Code Editor. IIRC this depends on the assetID of the script and there may be issues retrieving this if it's not explicitly defined part of the URL query string. I may be totally wrong on this

  • CDN
    This is more of a dev consideration, but ES modules will allow developers to import modules from a CDN using:

import moment from 'https://unpkg.com/moment'

There are obvious pro's/cons in relying on 3rd party CDN's at run time. Some issues being outages and also allowing non version locked modules which can suddenly be upgraded and subsequently introduce breaking changes for your app. This is not obviously something specific to ES modules per-se but the ability to import modules directly is likely to make the problem more prevalent. This is a persuasive argument for bundling 3rd party dependencies so you always have a static shippable build

@willeastcott willeastcott moved this from Todo to In Progress in PlayCanvas Roadmap Nov 8, 2022
@marklundin
Copy link
Member

I've just realized that scripts in the launch.playcanvas.com environment are actually hosted with the same paths as the asset registry. So relative import paths would work in this scenario, however downloaded projects and I assume published projects do not have the same paths, so the above issue with the import maps still stands

@kungfooman
Copy link
Collaborator

kungfooman commented Nov 21, 2022

import { attribute, inspectable } from 'playcanvas'

@inspectable("My Awesome Class", "This description is surfaced to the editor")
class MyClass {

  @attribute("This attribute title is exposed to the editor")
  nSomeValue= "This is a default value"

  initialize(){}
  destroy(){}
  update(){}
}

You loose the ability to ask for the default value without actually creating an instance? There is also an issue that PC fires events attr: events, so the property actually turns into a getter/setter internally.

I think ES6 is powerful enough for nice syntax + types already, e.g:

import * as pc from 'playcanvas';
import {
    ScriptType,
    Vec3,
    EVENT_KEYDOWN,
    KEY_SPACE,
} from 'playcanvas';
export class MouseInput extends ScriptType {
    static fromWorldPoint = new Vec3();
    static toWorldPoint = new Vec3();
    static worldDiff = new Vec3();
    /** @type {number} */
    orbitSensitivity;
    /** @type {number} */
    distanceSensitivity;
    static {
        this.attributes.add('orbitSensitivity', {
            type: 'number',
            default: 0.3,
            title: 'Orbit Sensitivity',
            description: 'How fast the camera moves around the orbit. Higher is faster'
        });
        this.attributes.add('distanceSensitivity', {
            type: 'number',
            default: 0.15,
            title: 'Distance Sensitivity',
            description: 'How fast the camera moves in and out. Higher is faster'
        });
    }
    // initialize code called once per entity
    initialize() {
        // <snip>
    }
}

The static initialization block is a ES2022 feature, but it can also be simulated like:

static _ = (
        this.attributes.add('orbitSensitivity', {
            type: 'number',
            default: 0.3,
            title: 'Orbit Sensitivity',
            description: 'How fast the camera moves around the orbit. Higher is faster'
        }),
        this.attributes.add('distanceSensitivity', {
            type: 'number',
            default: 0.15,
            title: 'Distance Sensitivity',
            description: 'How fast the camera moves in and out. Higher is faster'
        })
    );

Or of course just putting the attributes under the class declaration, but I think having it "inside" the class helps the code flow a bit.

Besides the import maps issue that depends on specific server setups / file distribution styles, isn't the current ES6 class system rather capable? What are the current pain points?

@kungfooman
Copy link
Collaborator

Thoughts on ES Modules

Module resolution Problem Module paths should resolve in a way synonymous to the asset registry. Unfortunately Import Maps won't work here as it apparently only allows you to map an import to a url so that:

import moment from 'moment'

becomes

import moment from 'https://unpkg.com/moment'

It doesn't allows you to modify the path resolution algorithm so that a relative path like '../../path/to/module' resolves to 'launch.playcanvas.com/api/assets/files/asset/script.js?id=123&branchId=231'

JavaScript devs do what they always do: polyfill

In this case you can use: https://github.com/guybedford/es-module-shims

By default it supports CSS and JSON loading via modules, but with a little rewrite, you can even make it load frag/vert e.g.

This was a limitation that triggered the change in e.g. this PR: #3850

So right now via es-module-shims you can do things like in most browsers:

import baseNineSliced from './baseNineSliced.frag';
import style from './style.css';

const preFrag = document.createElement('pre');
preFrag.innerText = baseNineSliced;
document.body.append('preFrag', preFrag);

const preStyle = document.createElement('pre');
preStyle.innerText = style;
document.body.append("style", preStyle);

To test for yourself, I made a little zip:
es-module-shims-example.zip

A programmable resolution hook

Some have suggested customizing the browser's module resolution algorithm using a JavaScript hook to interpret each module specifier.

Unfortunately, this is fatal to performance;

The polyfill is doing way more extra work than a programmable resolution hook and yet I barely feel that its shimmed (loading PC, PCUI, pc-observer all as modules + my extra code modules). So their performance argumentation is utter non-sense to me. Well, too bad that misconceptions of a few people drag down the power of the native web environment. At least we can always polyfill anyway, because JS itself is so powerful.

just wanted to point out that there is a "point three" in your list

@marklundin
Copy link
Member

Nice find on the es shims @kungfooman. Yup this is essentially what I meant by a pre-processing step that re-map or re-write imports. Although that probably wasn't clear 😆

  1. Pre-Processor. A build step that resolves and rewrites paths ahead-of-time or at run

You'd have to gauge the additional overhead incurred by rewriting all these imports at runtime tho, there's probably many cases where this isn't huge, but my guess is it wouldn't scale well to apps with large or complex dependency trees, and it would likely end up degrading the perceived load time. Definitely worth investigating though to measure actual impact, but in my mind any solution that results in an increased time to FCP should be avoided where possible, especially if it could be done ahead-of-time in a publish/build step or even better, avoided altogether.

I hadn't actually realized Import Maps weren't supported on FF or Safari, so you're right, in any case this sort of feature would need to be polyfilled at least over the short term.

@marklundin
Copy link
Member

Besides the import maps issue that depends on specific server setups / file distribution styles, isn't the current ES6 class system rather capable? What are the current pain points?

Yep you're right, the property decorator would of course require class instantiation. I guess the solution would be something like @attribute("Prop Title", "some default value") but to be honest, it is risky depending upon a JS feature set which is currently in draft status. This can probably be picked up again if/when it lands in stage 4.

@kungfooman
Copy link
Collaborator

@marklundin Yes, I see your point about First Contentful Paint, my idea is mostly to level Browser technology with bundlers. As soon it works in the browser the way devs are used to by e.g. rollup, you can kick out rollup for your development cycle for quick testing of ideas without worrying about outdated bundler artifacts etc... or the entire bundler setup in the first place. How many devs do just hate to figure out bundler bugs, issues and peculiarities?

My current setup loads 711 *.js module files at ~1s and with shim it takes ~1.5s to FCP, which is only for the slow ducks here (Safari / Firefox). Extra time also happens for Chrome, if you use natively unsupported functionality like importing vert/frag files.

The unshimmed module loading time also feels too long for me, so for a release build all of it should be just bundeled, tree-shaked, tersered and so on 😅. This is the time I actually want rollup: building a release when all the code is settled.

The good thing about ES5 is that you can just concat it all together, so in the context of PlayCanvas Editor there won't be a way around a more involved "module build step" for lowest FCP in any case or to accept the shim extra time until browsers catch up (for Safari I read Import Maps feature is in Technical Preview e.g.).

@kungfooman
Copy link
Collaborator

kungfooman commented Nov 26, 2022

I tested hot reloading a bit and it sorta works, just open http://127.0.0.1/playcanvas-test-es/es6.html and execute this in devtools:

const url = window.location.origin + '/playcanvas-test-es/es6/rotate.js';
const { Rotate } = await import(url + "?now=" + Date.now());
pc.registerScript(Rotate, Rotate.registerName);

"Sorta" because only the rotate.js will be updated, its dependencies are not updated, even with devtools open. I think it's the ES6 link mechanism itself at work, to prevent "double executions" and I didn't check too much yet if there is a way to clear the import cache.

I updated this repo that I created some time ago if someone wants to play around with it: https://github.com/kungfooman/playcanvas-test-es

(huge thanks to @LeXXik since I started this using his Ammo-Debug-Drawer)

@marklundin
Copy link
Member

marklundin commented Nov 26, 2022

Thanks for sharing @kungfooman. IIRC dynamic imports can't be statically analysed and are therefore ineligible for tree shaking. This is probably ok when developers explicitly use them, but not suitable at the editor/engine level.

My concern with the es import map polyfill is that the path resolution feature isn't part of the spec, so even when all browser support it, you'd still need the polyfill for the non-standard functionality. I'd still be keen to know the perf on a complex tree that makes heavy use of rewriting the import maps

@kungfooman
Copy link
Collaborator

Thanks for sharing @kungfooman. IIRC dynamic imports can't be statically analysed and are therefore ineligible for tree shaking. This is probably ok when developers explicitly use them, but not suitable at the editor/engine level.

Forget about dynamic import problems for Hot Code Reloading. The dynamic import is barely code that the PC Editor would trigger e.g. over WebSocket communication after a script is saved: to import the latest version and nothing else. Everything is and remains analyzable statically.

About the spec on https://github.com/WICG/import-maps#acknowledgments:

Since then, @guybedford has been instrumental in prototyping and driving forward discussion on this proposal.

The same guy who developed the es-module-shims. I think the spec is rather moldable and what browser vendors come up with is inspired by what JavaScript already do via shims/polyfills.

So basically if enough people rely on a feature, this is what browsers will eventually support natively (just like jQuery and document.querySelectorAll).

The spec will probably evolve a lot and then rather in a direction that is of use to most devs by informing browser vendors how JS developers would like/need it.

@MarshallBelles
Copy link

Excerpt from #924:

It would be great to have TypeScript support with code completion, syntax and error highlighting.
I personally think this is the most desirable missing feature from PlayCanvas.

Regarding code completion and error highlighting, I would take a look at https://github.com/coder/code-server if you don't know about it - extracting the already-proven VS Code browser is a no-brainer to me. I think you could still maintain complete control over the cloud editor IDE with minimal effort.

If you are open to a professional partnership then you should reach out to https://coder.com/ and see if they can help.
I'm not affiliated with them in any way, but I have used their workspaces.

Thanks, and cheers!

@kungfooman
Copy link
Collaborator

@MarshallBelles

The VS Code browser is called Monaco and PlayCanvas Editor is already using it.

image

You have everything that TS offers, but none of the downsides.

Q: What did the TypeScript developer say when asked about their code?
A: "It's there, but you can't see it for all the types!"

@marklundin
Copy link
Member

marklundin commented Dec 15, 2022

I agree with the need for TypeScript. I don't personally use it, but many people do. @ellthompson @yaustar I'd be keen to hear what the proposal is for this.

@yaustar
Copy link
Contributor

yaustar commented Dec 15, 2022

The current plan (subject to change) is to support modules by default and with that, support BOTH JS and TS workflows which will involve some sort of build step when opening the launch tab.

Again, still VERY early stages.

@marklundin
Copy link
Member

Super cool. I think that opens up a lot of potential. It would be great if there was some sort of semantic distinction between source and compiled files, so that source files could be attached to and associated with entities, but compiled scripts are not.

One of the issues we had with the Package Manger is that whilst I could compile scripts into a single bundle, they would lose the reference of which specific entity they were attached to

@yaustar
Copy link
Contributor

yaustar commented Dec 15, 2022

Oh that's interesting and not something that we've discussed

That said, with the new search system in the Editor that can search by script name, is that still needed?
image

@LeXXik
Copy link
Contributor

LeXXik commented Dec 15, 2022

@yaustar I do need it occasionally, but mostly in projects that were done by another developer, where I don't know where the script is attached to.

@yaustar
Copy link
Contributor

yaustar commented Dec 15, 2022

@LeXXik Doesn't the search help with that? Or is it more of a 'flow' when investigating stuff?

@LeXXik
Copy link
Contributor

LeXXik commented Dec 15, 2022

Yes, it does help with that. Was that the question? 😅 I mostly use it during investigations/debugging.

@yaustar
Copy link
Contributor

yaustar commented Dec 15, 2022

The question was if on the script asset, find references was needed/still needed if we have the search in hierarchy

@yaustar
Copy link
Contributor

yaustar commented Jan 3, 2023

Another question concern that has just popped up: At the moment, attribute parsing of scripts in the Editor is done on a per script asset basis. This has caused issues like playcanvas/editor#760 where constants or base classes are defined in other files.

With this system, would the attribute parsing take into account of the imports/exports or would it bundle the scripts first and then parse them?

@yaustar yaustar moved this from In Progress to Todo in PlayCanvas Roadmap Jan 19, 2023
@willeastcott willeastcott added this to the Q3 2023 milestone Jan 24, 2023
@mvaligursky mvaligursky assigned slimbuck and unassigned ellthompson Jun 5, 2023
@kungfooman
Copy link
Collaborator

kungfooman commented Jun 22, 2023

ES6 is forcing us to be more careful with scripts aswell. We cannot simply extend pc anymore, as it becomes a module:

image

image

image

The error is triggered when loading the WebXR VR Lab via ES6: https://developer.playcanvas.com/en/tutorials/webxr-vr-lab/

Probably possible to write a regex to warn PC Editor users of this anti-pattern (to make it future proof), or just let everyone figure that out for themselves with crashing/non-working scenes.

But what should be the suggested/better/working alternative?

Edit: simple regex would do: pc\.[a-zA-Z0-9_]+\s*=\s*

image

You simply can't assign anything:

image

This pattern also doesn't work:

image

@mvaligursky
Copy link
Contributor

We're facing the same issue with building engine examples using the es6 module version of the engine. @ellthompson has done some exploration, but I don't think we have a solution at the moment.

@kungfooman
Copy link
Collaborator

Technically you can just re-export the entire module:

<body></body>
<script>
  function importFile(content) {
    return "data:text/javascript;base64," + btoa(content);
  }
  const imports = {
    playcanvasOriginal: "/playcanvas-engine/src/index.js",
    playcanvas: importFile(`
      export * from "playcanvasOriginal";
      export class WhateverPeopleMiss {
          xyz = 123;
      }
    `)
  };
  const importmap = document.createElement("script");
  importmap.type = "importmap";
  importmap.textContent = JSON.stringify({imports});
  document.body.appendChild(importmap);
</script>
<script type="module">
  import * as pcOriginal from "playcanvasOriginal";
  import * as pc from "playcanvas";
  Object.assign(window, {
    pcOriginal,
    pc,
  });
</script>

Test:

image

So even though ES6 modules are frozen, we can basically just extend them anyway (by exporting everything from another module + our own stuff).

I had a look at the engine examples and I kinda don't like the way it evolved:

import { DEVICETYPE_WEBGL1, DEVICETYPE_WEBGL2, DEVICETYPE_WEBGPU } from '../../../build/playcanvas.js';

As example, it should simply be from 'playcanvas';. Every browser supports import maps these days, and bundlers are versatile enough to make it work as well.

And then the entire engine is already a proper MJS module, why do we need a build step at all? It seems overly complicated and confusing to me with all the extra steps.

@ThatStevenGuy
Copy link

Bit of a random bump, but we're also really in favor of the TypeScript decorator approach similar to what @marklundin suggested for declaring attributes and scripts. Here's an example of how we write our scripts:

code

Results in:

result

Declaring attributes this way feels pretty intuitive. It's somewhat reminiscent of Unity's SerializeField.

@marklundin
Copy link
Member

marklundin commented Jul 16, 2024

The decorator approach is nice, but it's still only a stage 3 proposal and we don't want to force people down a TS route.

We do however have something very similar using static JSDoc style tags to declare attributes. It works in a very similar way as your example, doesn't incur any runtime overhead and is compatible with vanilla JS. Feedback is welcome :D

@Maksims
Copy link
Contributor

Maksims commented Jul 17, 2024

Bit of a random bump, but we're also really in favor of the TypeScript decorator approach similar to what @marklundin suggested for declaring attributes and scripts. Here's an example of how we write our scripts:

Declaring attributes this way feels pretty intuitive. It's somewhat reminiscent of Unity's SerializeField.

This is very subjective and taste-dependent.
The amount of writing/learning curve to write attributes should be as small as possible, although it is not easy to tailor it due to our existing knowledge and overestimating the idea of "common knowledge" or even "common sense".

Also, the example you've provided above does not fully represent the UI you've shown, as in UI you have titles on fields, in code you don't. settings.setting1 - will be an ElementComponent or an Entity? As in Editor you select an entity, not a component. Same with settings.setting2, and what if you set entity in Editor, but then remove the "Bot" script from it? How does it behave?

It is best to avoid complex conditioning, magical rules and behaviours as much as possible.

@ThatStevenGuy
Copy link

I mean absolutely, it's our own personal taste. None of it is common sense and nowhere did I suggest that it is. We prefer to use a C# attribute style approach, because it allows us to attach information to fields which seems like a good fit for PlayCanvas script attributes. This is what I meant by it being intuitive: there's tight coupling between a decorator and the field it's attached to. The screenshot is meant to demonstrate how the traditional attribute declarations (i.e. someScript.attributes.add({args...})) can be merged into an @Attribute.declare({args...})-form (or something similar).

I'm going to preface this by saying that the screenshot is only supposed to demonstrate the use of attribute decorators in general. The additional features that our system supports are not relevant to the discussion here, so I understand the confusion. I do want to answer your questions though, so here it goes. Our system processes attributes in a way that we find helpful. Some of the things it does:

  • allow you to directly reference a component or script on another entity or template without needing to manually fetch them during a script's initialize call. The PlayCanvas Editor only allows for setting an entity, hence why the screenshot still show "Select Entity". If our system can't resolve the component or script at runtime, a warning is printed to the console.
  • automatically assign titles to attributes by removing leading characters and inserting spaces based on camel case / pascal case. It also capitalizes each word. We like nicely formatted labels for our attributes. Custom titles can still be supplied if desired.

Again, we are not suggesting that PlayCanvas should do things our way. We just like doing things this way.

I understand that TypeScript isn't for everyone and as such the decorator approach isn't a "must-have", but I wanted to weigh in that we find it very helpful, so maybe it can help steer the new attribute declaration style for the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

No branches or pull requests