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

ES6 and Object.defineProperties issue #3744

Open
treblereel opened this issue Dec 28, 2020 · 10 comments
Open

ES6 and Object.defineProperties issue #3744

treblereel opened this issue Dec 28, 2020 · 10 comments

Comments

@treblereel
Copy link
Contributor

treblereel commented Dec 28, 2020

lets say i have the following class:

class Object3D extends EventDispatcher {

	constructor() {

		super();

		const quaternion = new Quaternion();

		Object.defineProperties( this, {
			quaternion: {
				configurable: true,
				enumerable: true,
				value: quaternion
			}
		} );
      }

       onRotationChange() {
		this.quaternion.setFromEuler(........);
	 }
 }

and the following warning:

core/Object3D.js:123:7: WARNING - [JSC_INEXISTENT_PROPERTY] Property quaternion never defined on Object3D
  123| 		this.quaternion.setFromEuler();
       		   ^^^^^^^^^^

I have no idea what i do wrong, maybe i missed something ?

@ctjlewis
Copy link
Contributor

ctjlewis commented Dec 28, 2020

The compiler does not understand that this.quaternion is instantiated in the constructor. If you remove Object.defineProperties(...) and use this.quaternion = quaternion, this warning will go away.

Admittedly, it would be best if the compiler could understand the Object.defineProperties statement.

@treblereel
Copy link
Contributor Author

@ctjlewis thanks for your clarification, so it means closure ignores Object.defineProperties/Object.defineProperty ? Ok, i am working on three.js and i didn't want to change the code too much but looks like i have no choice to make it closure compatible.

@ctjlewis
Copy link
Contributor

Yeah, I think we have had issues about this before, for this exact library and this exact situation. @lauraharker, @brad4d, does this issue (with the Object.defineProperties conflicting with ThreeJS Quaternion class) also seem familiar? Also, thoughts on patching?

Some Googling gives me:

@nreid260
Copy link
Contributor

nreid260 commented Jan 6, 2021

If it's infeasible to update the actual property declaration to be a simple assignment, it would also work to declare the property separately. Something like /** @type {!Quaternion} */ Object3D.prototype.quaternion; would inform the compiler about the property with no runtime effect.

@treblereel
Copy link
Contributor Author

@nreid260 Thanks for advice, but it not able to do it, coz in my case Quaternion extends Object3D, @type {!Quaternion} needs me to add an export Quaternion in Object3D, that leads to :

Uncaught ReferenceError: Cannot access 'B' before initialization

:(

@nreid260
Copy link
Contributor

nreid260 commented Jan 7, 2021

How can Quaternion extend Object3D? Since the constructor of Object3D instantiates Quaternion, that would cause infinite recursion.

More to the point though,/** @type {!Quaternion} */ Object3D.prototype.quaternion; could be added anywhere in the program (even a separate file that just contains property declarations). If you chose to add it in the same file that defines Object3D that would also be fine. Quaternion is only referenced as a type (annotation) here, so this declaration is legal before Quaternion is defined. In compiler terms, this is a "forward reference".

As yet another option, if you're able to patch the code, you could replace the defineProperties call with /** @const {!Quaternion} */ this.quaternion = new Quaternion();. That would be the most idiomatic way to express the property to Closure Compiler. @const would give you very similar compile time guarantees to the ones the code has today by defining the property to be non-writable.

@treblereel
Copy link
Contributor Author

treblereel commented Jan 7, 2021

@nreid260 yeah, i know, it sounds strange, but it's very typical for three.js. Cycle deps everywhere ...

I am about to give up adopting three.js to closure, despite the fact, only 260 warnings left (initially it was ~3000) ...

thanks for your support !

@nreid260
Copy link
Contributor

nreid260 commented Jan 7, 2021

Depending on what the warnings are, you could just @suppress them. That's our standard approach to rolling out new diagnostics. The code works today, so the warnings likely don't correspond to real problems, and our optimizations generally behave safely (if less effectively) in response to issues.

@nreid260
Copy link
Contributor

Just want to add, the compiler recognizes properties creates by Object.defineProperties during optimizations, but not during typechecking. As such, we believe things are working as intended.

That distinction might feel arbitrary, but optimizations generally require less information about properties (usually just "does this name exist somewhere in the program?") so it's easier to add support. Recognizing them during typechecking is more nuanced.

Additionally, optimization failure can cause runtime behaviour issues, so we need to be very conservative, and often silently cancel optimizations when something might be wrong. In contrast, typechecking failure is a compile-time problem, so we can make it more user visible, and defer to the user (via suppressions or annotations) when we get false positives.

@treblereel
Copy link
Contributor Author

@nreid260

thanks, it works !

class Test {
    constructor() {
      Object.defineProperty( this, 'isTest', { value: true } );
      Object.defineProperties( this, { isTest2: { value: true } } );
    }
}

so isTest2 can be suppressed, but isTest is always undefined :(

class a{constructor(){Object.defineProperty(this,"isTest",{value:!0});Object.defineProperties(this,{a:{value:!0}})}};

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

No branches or pull requests

3 participants