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 Attribute events #6913

Open
marklundin opened this issue Sep 5, 2024 · 6 comments
Open

ESM Attribute events #6913

marklundin opened this issue Sep 5, 2024 · 6 comments
Assignees

Comments

@marklundin
Copy link
Member

marklundin commented Sep 5, 2024

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
Copy link
Contributor

What would it do under the hood?

@marklundin
Copy link
Member Author

marklundin commented Sep 5, 2024

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
Copy link
Contributor

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

@marklundin
Copy link
Member Author

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
Copy link
Contributor

Maksims commented Sep 5, 2024

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
Copy link
Member Author

marklundin commented Sep 5, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants