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

Use Object.assign for constant definitions. #8838

Merged
merged 1 commit into from
May 7, 2016

Conversation

tschw
Copy link
Contributor

@tschw tschw commented May 7, 2016

See #8824

@mrdoob mrdoob merged commit bfd833f into mrdoob:dev May 7, 2016
@mrdoob
Copy link
Owner

mrdoob commented May 7, 2016

Thanks!

@mrdoob
Copy link
Owner

mrdoob commented May 7, 2016

As far as I understand, this pattern:

THREE.RingGeometry.prototype=Object.create(THREE.Geometry.prototype);
THREE.RingGeometry.prototype.constructor=THREE.RingGeometry;

Can now be just this...

Object.assign( THREE.RingGeometry.prototype, THREE.Geometry.prototype );

Right?

@tschw
Copy link
Contributor Author

tschw commented May 7, 2016

@mrdoob

Object.assign( THREE.RingGeometry.prototype, THREE.Geometry.prototype );

Unfortunately it's not quite that simple: In case we use this scheme, we get false asking whether aRingGeometry instanceof THREE.Geometry, because

THREE.RingGeometry.prototype.constructor === THREE.RingGeometry &&
THREE.RingGeometry.prototype.__proto__.constructor  === Object

and not

THREE.RingGeometry.prototype.constructor === THREE.RingGeometry &&
THREE.RingGeometry.prototype.__proto__.constructor === THREE.Geometry &&
THREE.RingGeometry.prototype.__proto__.__proto__.constructor  === Object

.

So it's

THREE.RingGeometry.prototype =
        Object.assign( Object.create( THREE.Geometry.prototype ), {

    constructor: THREE.RingGeometry, // else it's THREE.Geometry

    /* ... */

} );

.

One could change .prototype.__proto__, but the MDN documentation reads:

Warning: Changing the [[Prototype]] of an object is, by the nature of how modern JavaScript engines optimize property accesses, a very slow operation, in every browser and JavaScript engine. The effects on performance of altering inheritance are subtle and far-flung, and are not limited to simply the time spent in obj.__proto__ = ... statement, but may extend to any code that has access to any object whose [[Prototype]] has been altered. If you care about performance you should avoid setting the [[Prototype]] of an object. Instead, create a new object with the desired [[Prototype]] using Object.create().

Warning: While Object.prototype.__proto__ is supported today in most browsers, its existence and exact behavior has only been standardized in the ECMAScript 6 specification as a legacy feature to ensure compatibility for web browsers. For better support, it is recommended that only Object.getPrototypeOf() be used instead.

So that's not really an option for us.

You can get around Object.create like done here, however, I have no clue why they actually do it (I believe this code may only be used when the "debug loader" is in effect - when compiled, the Closure compiler actually generates code that uses Object.create unless it can squash the inheritance altogether and if I'm not mistaken).

@mrdoob
Copy link
Owner

mrdoob commented May 7, 2016

I see I see. As usual, thanks a ton for the explanations!

@tschw
Copy link
Contributor Author

tschw commented May 7, 2016

I had to correct above post; obj.prototype isn't meaningful. It's either ctor.prototype or obj.__proto__.

@tschw tschw deleted the AssignConstants branch May 8, 2016 03:14
mrdoob added a commit that referenced this pull request May 8, 2016
mrdoob added a commit that referenced this pull request May 8, 2016
mrdoob added a commit that referenced this pull request May 8, 2016
carlosanunes pushed a commit to carlosanunes/three.js that referenced this pull request May 18, 2016
carlosanunes pushed a commit to carlosanunes/three.js that referenced this pull request May 18, 2016
carlosanunes pushed a commit to carlosanunes/three.js that referenced this pull request May 18, 2016
carlosanunes pushed a commit to carlosanunes/three.js that referenced this pull request May 18, 2016
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

Successfully merging this pull request may close these issues.

2 participants