Tree-shaking broken by Object.defineProperties( ... )

[donmccurdy]

Unclear to what extent this is a change in bundlers, or a misunderstanding of how tree-shaking applies, but the three.module.js bundle mostly doesn't get tree-shaken as expected today. Here's a very minimal repro of the issue in Rollup, but we think something similar happens with esbuild and webpack too:

Rollup Repl

References:

• https://twitter.com/MarcoGomezGT/status/1520866649485754372
• Treeshaking is defeated by Object.defineProperty rollup/rollup#1130
• Tree-shaking doesn't work when there is Object.defineProperty in a class rollup/rollup#3771

[marcofugaro]

You can test tree-shaking directly inside the tree.js repo by running

npm run build && npm run test-treeshake

You can comment things in src/Three.js to see what's going on, the minimal example seems not to work with the three.js classes, doing more tests...

[donmccurdy]

Ideas in no particular order:

(a): rewrite Three.Legacy.js to avoid Object.defineProperties

Might mean we can no longer warn about access to old properties with getters, only warn about methods?

Vector3.prototype.getFoo = function () {

  console.warn( ... );

}

(b): publish production/development builds

E.g. provide three.production.module.js, omit Three.Legacy.js from these builds, and find a practical way to support bundlers with builds published to npm:

if ( process.env.ENV === 'development' ) {

  Object.defineProperties( Vector3.prototype, { ... } );

}

(c): provide legacy warnings through a separate export

import { Vector3 } from 'three'; // no legacy warnings
import 'three/legacy'; // see all the legacy warnings, but lose tree-shaking

console.log( new Vector3() );

[LeviPesin]

I think that the lines like https://github.com/mrdoob/three.js/blob/dev/src/core/Object3D.js#L66-L93 are also not tree-shakable, but not related to the Three.Legacy...

[donmccurdy]

If Object.defineProperties is in the constructor, that seems OK as far as rollup is concerned, but I haven't done tests in other bundlers.

[marcofugaro]

@donmccurdy I have a repro of the .prototype.is* issue I was saying on twitter:

1. open mrdoob/three.js locally
2. replace the src/Three.js contents with:

// minimal example
export { PerspectiveCamera } from './cameras/PerspectiveCamera.js';
export { Scene } from './scenes/Scene.js';
export { WebGLRenderer } from './renderers/WebGLRenderer.js';

// unused class
export { InstancedBufferGeometry } from './core/InstancedBufferGeometry.js';

3. run npm run build && npm run test-treeshake
4. open test/treeshake/index.bundle.js

As you can see the class InstancedBufferGeometry is included even if actually unused.

Now comment this line

three.js/src/core/InstancedBufferGeometry.js

Line 44 in ce66f72

InstancedBufferGeometry.prototype.isInstancedBufferGeometry = true;

And run again npm run build && npm run test-treeshake

You can see that the unused InstancedBufferGeometry class is correctly tree-shaken.

[pschroen]

Same, I also confirmed @marcofugaro's point this morning, tree-shaking behaves differently when importing from node_modules. In the tests I did all the .prototype.is* properties cause the class to be included in the final bundle whether you are using it or not, so essentially the entire library is bundled regardless.

For example removing this one line of code makes a difference of nearly 100 KB of files that get bundled.

three.js/src/animation/AnimationMixer.js

Line 766 in ce66f72

AnimationMixer.prototype._controlInterpolantsResultBuffer = new Float32Array( 1 );

[pschroen]

@donmccurdy @marcofugaro to summarize I think we're dealing with at-least three problems in this issue that would need to be addressed:

1. Object.defineProperties outside of the classes need to be removed. Inside the class is fine.
2. .prototype.is* properties outside of the classes need to be removed. Suggesting the same as @marcofugaro did on Twitter with this.is* inside the constructor.
3. Alternative solution for handling the API changes warnings, other frameworks or libraries handle this with @deprecated decorators and keeping the method warnings inside the classes themselves.

All this to say, in my experience the only true way to have a library fully tree-shakeable is the classes all need to be self-contained, without any side-effect artifacts referencing them, and only imported by other classes that use them.

[marcofugaro]

3. Alternative solution for handling the API changes warnings, other frameworks or libraries handle this with @deprecated decorators and keeping the method warnings inside the classes themselves.

This was discussed in the past, not really feasible and easy to maintain according to the core maintainers.

[pschroen]

Ya was expecting that to be the case, that's the trade-off though, more difficult to maintain with messier classes. Perhaps there's some other way we could rewrite Three.Legacy.js?

Not a fan of the additional endpoints and modules myself, would rather keep everything imported from 'three' if we can! 😉

[donmccurdy]

I have a repro of the .prototype.is* issue I was saying on twitter...

Currently failing for me, tested with Node.js v12.22.12, v14.19.2, and v17.5.0 —

> rollup -c test/rollup.treeshake.config.js

[!] Error: While loading the Rollup configuration from "test/rollup.treeshake.config.js",
Node tried to require an ES module from a CommonJS file, which is not supported. A common 
cause is if there is a package.json file with "type": "module" in the same folder. You can 
try to fix this by changing the extension of your configuration file to ".cjs" or ".mjs" 
depending on the content, which will prevent Rollup from trying to preprocess the file but 
rather hand it to Node directly.

https://rollupjs.org/guide/en/#using-untranspiled-config-files

Error: While loading the Rollup configuration from "test/rollup.treeshake.config.js", 
Node tried to require an ES module from a CommonJS file, which is not supported. A 
common cause is if there is a package.json file with "type": "module" in the same 
folder. You can try to fix this by changing the extension of your configuration file 
to ".cjs" or ".mjs" depending on the content, which will prevent Rollup from trying 
to preprocess the file but rather hand it to Node directly.

[looeee]

ideas in no particular order:
(a): rewrite Three.Legacy.js to avoid Object.defineProperties
(b): publish production/development builds
(c): provide legacy warnings through a separate export

Ya was expecting that to be the case, that's the trade-off though, more difficult to maintain with messier classes. Perhaps there's some other way we could rewrite Three.Legacy.js?

An option (d): switch to semver, release a version 1.0.0 with all the legacy code removed, and take care of breaking changes in a more standard way going forwards. Few if any other libraries support backwards compatibility in the way that three.js does. Besides problems like this I think it also leads to "ugrade-itis" where users expect they should bump the three.js version number every month and from personal experience I know that's a bad idea and I don't thing it's something we need to, or should encourage.

I know has been suggested and rejected before, but given that by now it's highly likely most people use three.js via NPM, perhaps it's time to revisit this? There's a standard method for updating NPM packages and three.js is one of the very few libraries that doesn't follow it, since we are releasing a new "minor" version every month. Minor versions are not supposed to contain breaking changes, however any three.js release may contain breaking changes.

In practice, most three.js releases don't contain breaking changes so the majority of new releases would still be minor or patch releases, we could still stick with the monthly release schedule. The differences would be a) we remove all legacy code and b) we need to consider on release day whether the release will be major (breaking changes), minor (non-breaking changes), or patch (just bug fixes).

I realize this is somewhat tangential to the current issue although it would solve one of the three problems @pschroen highlighted. If there's interest in discussing it further I'll move it to a new issue.

[marcofugaro]

Currently failing for me, tested with Node.js v12.22.12, v14.19.2, and v17.5.0 —

Looks like a node issue.. try maybe Node 18.0.0? It is working for me with that version. Remember to rm -rf node_modules when you change version.

[LeviPesin]

most three.js releases don't contain breaking changes

That is just incorrect. Every version of three.js contains breaking changes.

[Mugen87]

I suggest to not use this issue for discussions about the release cycle and versioning of three.js. Changes in that context are not required to improve tree-shaking.

Regarding #24006 (comment), I think we can solve the first point by just removing all Object.defineProperties statements from Three.Legacy.js. At all other places in the code base (core and examples), Object.defineProperties calls are already inside classes.

The mentioned code in Three.Legacy.js can be removed since:

• most of the fallbacks are several years old.
• in general it's recommended to avoid large upgrade steps (e.g. r70 -> r140) but do it in a more incremental fashion instead (e.g. r70 -> r80 -> r90)
• removing the code would allow us to decrease the file size of the core builds a bit.

For the future, I indeed suggest to keep deprecated properties in the respective classes for a certain period of time (e.g. one year). After that time, they can be removed.

[pschroen]

@Mugen87 I agree, also on discussion for the versioning being a different issue. It is related to this issue though, as pointed out by @looeee and a possible option for removing Three.Legacy.js.

It's a complicated subject though and personally I'm on the fence about semver for three.js, lots of pros/cons. 😅 cc @mrdoob 👇

https://twitter.com/mrdoob/status/1521672283831603208