playcanvas / engine

JavaScript game engine built on WebGL, WebGPU, WebXR and glTF
https://playcanvas.com
MIT License
9.59k stars 1.34k forks source link

ESM Attribute events #6913

Open marklundin opened 3 weeks ago

marklundin commented 3 weeks ago

ESM Scripts by design do not trigger attr:{propName} events. In order to help migration, it would be helpful to provide some alternative. Something like;

import { Script, watch } from 'playcanvas';

class MyClass extends Script {
    prop = 10;

    constructor() {
        watch(this, 'prop');
        this.on('change:prop', console.log);
    }
}

This allows user to attach events and can be applied to any class member, whether an attribute or not.

Any thoughts/input appreciated. @mvaligursky @willeastcott @Maksims @slimbuck

mvaligursky commented 3 weeks ago

What would it do under the hood?

marklundin commented 3 weeks ago

I'd guess something similar to the current ScriptType, but it would be explicit, optional and applicable to non attributes too.

const watch = (target, prop) => {
    const privateProp = `#{prop}`;
    target[privateProp] = target[prop];

    Object.defineProperty(target, prop, {
        set(value) {
            if (target[privateProp] !== value) {
                target.fire(`changed:${prop}`, value);
                target[privateProp] = value;
            }
        },
        get() {
            return this[privateProp];
        }
    });
}
mvaligursky commented 3 weeks ago

Feels kinda useful, even though easy enough and cleaner to do by the user. Would the initial value be lost with this implementation?

marklundin commented 3 weeks ago

Would the initial value be lost with this implementation?

We could just assign this , have updated the implementation

Feels kinda useful, even though easy enough and cleaner to do by the user.

Yep arguably we could just include this as a reference in the docs, as opposed to providing an implementation and supporting all the edge cases.

Maksims commented 3 weeks ago

Often multiple properties are watched, and sometimes all of them:

this.on('attr', (prop, value) => {
    //   
});

So if the goal is to provide API similarity, then try to mimic it as much as possible, e.g. instead of change:{prop} use same notation: attr:{prop}, and provide a global attr event also.

The ugly part is to write: this.watch(this, 'prop') multiple times, and the worst part of it - if watch used outside of constructor, it will likely deopt classes, as you use defineProperty on the class instance, so JS engines might struggle to build optimized shadow-class (machine optimization when classes have consistent number and types of properties).

marklundin commented 3 weeks ago

Yep the V8/etc deopt is a valid point. It may be better for users to handle this manually through class getters/setters