engine icon indicating copy to clipboard operation
engine copied to clipboard

ESM Attribute events

Open marklundin opened this issue 1 year ago • 6 comments

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

marklundin avatar Sep 05 '24 13:09 marklundin

What would it do under the hood?

mvaligursky avatar Sep 05 '24 13:09 mvaligursky

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];
        }
    });
}

marklundin avatar Sep 05 '24 13:09 marklundin

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

mvaligursky avatar Sep 05 '24 13:09 mvaligursky

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.

marklundin avatar Sep 05 '24 14:09 marklundin

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

Maksims avatar Sep 05 '24 16:09 Maksims

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

marklundin avatar Sep 05 '24 16:09 marklundin