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

ScriptType does not support class syntax #6316

Closed
AlexAPPi opened this issue Apr 30, 2024 · 4 comments · Fixed by #6367
Closed

ScriptType does not support class syntax #6316

AlexAPPi opened this issue Apr 30, 2024 · 4 comments · Fixed by #6367
Assignees
Labels

Comments

@AlexAPPi
Copy link
Contributor

AlexAPPi commented Apr 30, 2024

Script argument type conversion does not occur if both a property and a script attribute exist.

    class TestFailVec3 extends pc.ScriptType {
        vec;
        initialize() {
            console.log(this.vec);
        }
    }
    pc.registerScript(TestFailVec3, 'TestFailVec3');
    TestFailVec3.attributes.add('vec', { type: 'vec3', default: [0, 100, 0] });
    class TestOkVec3 extends pc.ScriptType {
        initialize() {
            console.log(this.vec);
        }
    }
    pc.registerScript(TestOkVec3, 'TestOkVec3');
    TestOkVec3.attributes.add('vec', { type: 'vec3', default: [0, 100, 0] });

image

@AlexAPPi
Copy link
Contributor Author

This also applies to other complex types: vec2, entity, ...

@marklundin
Copy link
Member

Thanks @AlexAPPi, you've hit on an important point.

When an ES6 class members is defined, it is non configurable. Currently ScriptAttribute attempts to redefine the member using Object.defineProperty which effectively becomes a no-op. This prevents the type conversion and attribute events from working.

class Test { fail = 'should be overridden' }
Object.defineProperty(Test.prototype, 'fail',  { value: 100 });
test = new Test()
console.log(test.fail) // 'should be overridden'

I've created an editor repro here

For the time being, you'll need to avoid declaring attribute in the class body, or alternatively use the var MyClass = pc.createScript syntax. We plan on solving this limitation with the newer ESM Scripts

@marklundin marklundin changed the title Type conversion does not occur if both a property and a script attribute exist. ScriptType does not support class syntax Apr 30, 2024
@LeXXik
Copy link
Contributor

LeXXik commented Apr 30, 2024

At the moment, the issue can be very obscure, as it doesn't warn the developer. This can lead to bugs that are hard to debug. We should add a warning.

@marklundin
Copy link
Member

marklundin commented Apr 30, 2024

Yep this is not ideal, but unfortunately I don't think this is possible to catch at run-time without instantiating the class first. A solution is to prevent users using the class notation or declaring members in the class body. Neither are ideal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants