Src: Removed Object.defineProperty() calls.

[mrdoob]

Fixes: #21284

Description

@marcofugaro but... as you said, this test will now fail?

And doing Vector3.prototype.isVector3 = true breaks tree shaking.

🤷‍♂️

[marcofugaro]

@mrdoob do not despair! I am here to help.

Just replace these lines

three.js/test/unit/src/core/Object3D.tests.js

Lines 300 to 304 in 1e210ac

	assert.propEqual( obj.position, {
	x: 1,
	y: 1.23,
	z: - 4.56
	} );

with this

assert.numEqual( obj.position.x, 1, 'x is equal' );
assert.numEqual( obj.position.y, 1.23, 'y is equal' );
assert.numEqual( obj.position.z, - 4.56, 'z is equal' );

And the tests are fixed. I tested this branch locally and there doesn't seem to be any other test that break.

[mrdoob]

@marcofugaro thanks!

[mrdoob]

It's still annoying that we can't just do Vector3.prototype.isVector3 = true.
Before all Vector3s would share a single boolean. Now we're creating a boolean per Vector3 😕

[DefinitelyMaybe]

I agree that's not optimal. I had thought that we'd be able to make use of static properties and methods. progress is slow.

[DefinitelyMaybe]

There is also doing:

class Box2 {

	isBox2 = true;

	constructor( min, max ) {

		this.min = ( min !== undefined ) ? min : new Vector2( + Infinity, + Infinity );
		this.max = ( max !== undefined ) ? max : new Vector2( - Infinity, - Infinity );

	}

i.e. moving items not dependent on constructor params outside of the constructor

[mrdoob]

@DefinitelyMaybe #19986 (comment)

[Mugen87]

Before all Vector3s would share a single boolean. Now we're creating a boolean per Vector3 😕

We should be aware that iterating with a for in loop and then checking properties via hasOwnProperty() will produce a different (from my point of view unexpected) result now.

r125: https://jsfiddle.net/dstwvfg6/1/
r126dev: https://jsfiddle.net/gdj89m7t/

I wonder if the performance penalty mentioned in #21284 is not preferable compared to the change in behavior. Especially since the performance drop should not be visible if clone() is used properly.

[mrdoob]

@marcofugaro Are you sure that Vector3.prototype.isVector3 = true breaks tree-shaking?

I just did a test and it seems like it works just fine: f0ad404

I've also tried Object.assign( Vector3.prototype, { isVector3: true } ); instead and that definitely breaks tree-shaking.

[DefinitelyMaybe]

question mark?

if tree-shaking is mostly surrounding import/export's then neither
Vector3.prototype.isVector3 = true or Object.assign( Vector3.prototype, { isVector3: true } );
should break tree-shaking, but one reportedly does above.

I'd still prefer not to go back to either.

I'd submit that keeping data related to an object inside of that object to be more beginner friendly than putting that same data somewhere within the file related to the object.

don't mind me. I forgot this thing.

was the a verdict regarding static properties yet?

class Object3D {

	static isObject3D = true
	// ...

}

Static properties are not a substitute for this because they are not accessible via the instance of that object

[mrdoob]

was the a verdict regarding static properties yet?

Static properties are not a substitute for Vector3.prototype.isVector3 = true.

[DefinitelyMaybe]

oops. I'd forgotten that discussion.

[DefinitelyMaybe]

I should google some things before asking

quick question, why is `prototype` used? couldn't you just do `Vector3.isVector3 = true` ?

[DefinitelyMaybe]

@mrdoob re: earlier comment

I changed Vector2.js to:

class Vector2 {

	x;
	y;
	isVector2 = true;

	constructor( x = 0, y = 0 ) {
		this.x = x;
		this.y = y;
	}

and this was preserved within the three.module.js but not within the three.js

edit:
oh, basically you'd like babel to convert the syntax above to something like:

var Vector2 = /*#__PURE__*/function () {
	function Vector2(x, y) {
		if (x === void 0) {
			x = 0;
		}

		if (y === void 0) {
			y = 0;
		}
		
		this.x = x;
		this.y = y;
	}

	var _proto = Vector2.prototype;

	_proto.isVector2 = true;

within the three.js build

[marcofugaro]

@marcofugaro Are you sure that Vector3.prototype.isVector3 = true breaks tree-shaking?

It breaks it only for webpack but not rollup unfortunately 🙈

I added webpack to the script test-treeshake in my branch here marcofugaro@820c625.

I took DodecahedronGeometry as an example since it's not used anywhere else, unlike Vector2 or Vector3.

After adding this line:

You can see it's present in the webpack bundle but not the rollup one:

index.bundle.js	index-webpack.bundle.js

I tested the webpack bundle before and after #21293 and it looks like the bundle size has gotten bigger:

index-webpack.bundle.js before #21293	index-webpack.bundle.js after #21293

I'd vote for #21293 to be reverted. In my opinion, doing this.isVector3 = true is fine since a boolean is just a boolean, it wouldn't take as much memory space as an object would, for example.

[Mugen87]

I'd vote for #21293 to be reverted. In my opinion, doing this.isVector3 = true is fine since a boolean is just a boolean, it wouldn't take as much memory space as an object would, for example.

Sorry, but I don't vote to do that. Like mentioned before the property iteration behavior is changed when doing this which can be very confusing and a breaking change for apps.

[mrdoob]

Agreed. I vote for reporting this as a bug/feature request to Webpack.

[mrdoob]

The .is* properties are supposed to be internal. Polluting the public API with them is ugly and confusing.

Invites people to question why some classes have them but not others? And they should just not know about them

[mrdoob]

Trivia: Rich Harris himself added them 🤓

[mrdoob]

TIL a quick way to test if a module tree-shakes:

echo "import { Vector3 } from './src/math/Vector3.js'" | npx rollup -f es

[mrdoob]

I vote for reporting this as a bug/feature request to Webpack.

Done: webpack/webpack#12716