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

Core: make Vector and Matrix .is* property non-enumerable #24219

Merged
merged 1 commit into from
Jun 13, 2022

Conversation

marcofugaro
Copy link
Contributor

@marcofugaro marcofugaro commented Jun 9, 2022

Related issue: Fixes #24167 as proposed by #24167 (comment)

Description

Since we decided not to support .toJSON() for Vector and Matrix classes (#24172), let's support the JSON.stringify() serialization.

Setting the prototype in the constructor allows for tree-shaking and makes the .isVector3 property non-enumerable, so it doesn't show up in the JSON.stringify().

This does not make a breaking change as opposed to the instanceof method 😬

@mrdoob
Copy link
Owner

mrdoob commented Jun 10, 2022

Interesting...

This does not make a breaking change as opposed to the instanceof method 😬

I'm curious about how much of a breaking change that'd be though...

@marcofugaro
Copy link
Contributor Author

marcofugaro commented Jun 10, 2022

I'm curious about how much of a breaking change that'd be though...

You mean if people are using .isVector3? I don't really know but I believe some people out there might have it deep in their code haha

@Dannie226
Copy link
Contributor

If anything, people would be using the .isVector3 property for type checking. Given that checking a boolean would probably be faster in js than comparing two objects, I tend to prefer using the .is* properties, but, it is a simple change (find .isVector3, replace with instanceof Vector3/THREE.Vector3).

@mrdoob
Copy link
Owner

mrdoob commented Jun 13, 2022

Alright. Lets go with this.
However, this wins the most-weird-thing-in-the-library award imho 😁

@mrdoob mrdoob added this to the r142 milestone Jun 13, 2022
@mrdoob mrdoob merged commit c14b718 into mrdoob:dev Jun 13, 2022
@mrdoob
Copy link
Owner

mrdoob commented Jun 13, 2022

Thanks!

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.

isVector3 is present when serializing Vector3
3 participants