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

[pschroen]

Adding some more detail here from this morning's troubleshooting, and possibly a fourth problem.

The dead end I hit was on /*@__PURE__*/ code outside of the classes, most of the code outside the classes don't have the pure annotation, it's not always needed, however I tried adding /*@__PURE__*/ to test, but some of the code outside the classes are doing stuff, in other words not just declarations.

So current tree-shaking techniques mark them non-pure whether they have the pure annotation or not. Here's just one example of code being bundled when not in use.

three.js/src/animation/PropertyBinding.js

Line 2 in 0616013

const _RESERVED_CHARS_RE = '\\[\\]\\.:\\/';

And adding the pure annotation still doesn't work because the code is actually being used, and that's how tree-shaking works, so it's working properly here.

Input:

const _RESERVED_CHARS_RE = /*@__PURE__*/ '\\[\\]\\.:\\/';
...
const _wordCharOrDot = /*@__PURE__*/ '[^' + _RESERVED_CHARS_RE.replace( '\\.', '' ) + ']';

Output:

const _RESERVED_CHARS_RE =  '\\[\\]\\.:\\/';
'[^' + _RESERVED_CHARS_RE.replace( '\\.', '' ) + ']';

Note that all these issues are AST parsing of code outside the classes. When tree-shaking is done on an entry point file like ./src/Three.js there's none of that, the bundler just imports the named classes/objects, and it's done. That's why it works from ./src/Three.js.

For reference here's the Tree Shaking documentation from Webpack, I used Rollup for my tests but they handle the pure annotations the same, and so far the results seem to be the same across bundlers, so this would appear to be more of a three.js problem with how the code is being output if we are to continue using the flat three.module.js bundle as the entry point for the library. 😬

https://webpack.js.org/guides/tree-shaking/

[pschroen]

So I'm going to propose something here that may seem radical to some, but it's not really in 2022, and I believe it's "the path of least resistance" here.

Let me know if you guys agree but I view three.js as serving two use cases, as a three.js "application", and as a three.js class "library". For example I've personally used three.js on non-WebGL projects just to import from the internal math classes, I know I'm not the only one who does this.

• An "application" would need the usual code bootstrap stuff like REVISION, __THREE_DEVTOOLS__, the check for multiple instances and API warnings.
• A "library" would arguably just need the API warnings.

What I'm proposing here is that three.js finally moves to a real ES module library, that would mean removing all CommonJS and Rollup, no bundling, just the separate classes. Even for the examples, loading them from ./src/Three.js works fine and not as slow as you may think.

It would require two pretty major changes though:

1. Remove CommonJS support and Rollup flat file bundling completely, pointing the package.json entry point and all examples to ./src/Three.js. Just "type": "module", "main": "./src/Three.js", "sideEffects": false is all that's needed.
2. As @Mugen87 suggested move only the most recent method API warnings to their respective classes, and you can still keep Three.Legacy.js but just for deprecated classes which will work fine with tree-shaking.

That's it!

I know simpler said than done though right? If you guys are up for it I'm happy to help with this work. 😅

And dare I say, maybe this would be a big enough change to justify a switch to semver as well! 👀

[donmccurdy]

... three.js finally moves to a real ES module library, that would mean removing all CommonJS and Rollup, no bundling, just the separate classes

Throwing out the build feels like a last-resort option to me. Rollup is a popular and well-respected tool, we're squarely in the use case it's made for, and the maintainers clearly don't see bundles and tree-shaking as incompatible goals. Our builds do much more than just concatenate files, all of which would be lost by publishing raw source files.

I'm really hoping we can get a statement from a Rollup or Webpack maintainer about what's going on and how we or they could fix it. If the conclusion is really "bundlers are bad for libraries", then that's a huge deal for the wider JS ecosystem. It's not that we couldn't get by without Rollup, but something feels wrong if it's just three.js coming to the conclusion that this is necessary.

(I have no opinion on switching to semver, removing Three.legacy.js, or removing CommonJS builds at this time.)

[pschroen]

That's fair, and I understand.

Just wanted to put that out there as a pure ES module approach myself and @gordonnl have been doing with our work and used by OGL. It's been possible ever since Node.js added ES module support, and I would argue that's how ES modules were intended to be used, but that's for a different thread. 😅

So if we're aligned on keeping the flat three.module.js bundle as the entry point for the library then the path is clear, and we know what to do. The above points all still stand and we can make the bundle tree-shakeable by minimizing the amount of code that's executed outside of the classes.

I do still feel there is merit in changing the entry point to ./src/Three.js, @Rich-Harris seems in favour of that as well and avoids all these problems.

He also suggested agadoo which we should checkout. 🙂

[pschroen]

And speaking of agadoo, spent some time tonight testing it out (nice one @Rich-Harris 🙌), as you pointed out.

What it doesn't do
Tell you why tree-shaking fails

Failed to tree-shake build/three.module.js

From the README.md:

Avoid transpilers like Babel and Bublé (and if you're using TypeScript, target a modern version of JavaScript)
Export plain functions
Don't create instances of things on initial evaluation — instantiate lazily, when the functions you export are called

I don't like being the guy stirring the pot over all this stuff, though "Export plain functions" would include things like namespaces, so things like THREE.* and MathUtils.*. I realize this was common practice back in the day, but since ES modules, things have moved towards named imports now, like in the original post that started this conversation by @MarcoGomezGT.

Here's also a good reference on this topic by @bluepnume. 😉

https://bluepnume.medium.com/javascript-tree-shaking-like-a-pro-7bf96e139eb7

[looeee]

If the conclusion is really "bundlers are bad for libraries", then that's a huge deal for the wider JS ecosystem. It's not that we couldn't get by without Rollup, but something feels wrong if it's just three.js coming to the conclusion that this is necessary.

Yeah, it seems much more likely that it's something to do with the way three.js is written so we should thoroughly investigate that first (albeit I kinda like @pschroen's radical ideas and feel they warrant consideration, although I don't know right now what side I would come down on). Agadoo seems like a good starting point for the investigation.

Of the things mentioned here, the only things we do that seem unusual are keeping so much legacy support, and the .prototype.is* properties. For the latter, is there another way we can handle this? Modifying prototypes feels kind of archaic when using ES6 classes and the behavior is confusing as @donmccurdy pointed out on Twitter. It would be better to ban any prototype modification in src/, if possible.

Looking through src/, beside the .prototype.is* and Three.Legacy.js, I think prototype modification is only done in the animation system (@bhouston this is a coincidence I swear, nothing to do with our Twitter conversation from this morning 😅). Has anyone tested whether that that gets tree-shaken?

Regarding the use of Object.defineProperties outside of classes, we might need to move the relevant legacy code directly into the classes. Kind of ugly but not technically a problem I think.

Regarding semver, one final point and then I won't mention it again here because I don't want to sidetrack the discussion (even more). The reason I am in favor of switching is because we already do use semver. We publish on NPM and it's not possible to do that without using semver, so we're already using it. We just do so wrongly and should fix that if we can since NPM is presumably where the large majority of devs get three.js from these days.

[mrdoob]

Regarding Three.Legacy.js...

Maybe one way to deal with it would be by only keeping the helper code for 10 releases.
To make this easier, any time we add a helper we should add a comment on top specifying the release it was introduced.

If we adopted that, I suspect most of the file (if not all?) would be gone.

[LeviPesin]

and the .prototype.is* properties

I think we can safely use instanceof instead of them?

[Mugen87]

Maybe one way to deal with it would be by only keeping the helper code for 10 releases.
To make this easier, any time we add a helper we should add a comment on top specifying the release it was introduced.

Sounds good to me. There should be definitely a temporal limit for keeping legacy code. 10 releases (which is almost a year) feels long enough.

[mrdoob]

@LeviPesin

I think we can safely use instanceof instead of them?

instanceof is even worse for tree-shaking.
We used it in the past but Rich Harris himself changed all to .prototype.is*.

[mrdoob]

@Mugen87

Sounds good to me. There should be definitely a temporal limit for keeping legacy code. 10 releases (which is almost a year) feels long enough.

Alright, let's do it 👍
Would you like to take care of it?

[LeviPesin]

instanceof is even worse for tree-shaking.

Hm, but why?

[Mugen87]

Would you like to take care of it?

Yes. I'll remove everything below r130. For all other entries in THREE.Legacy.js I will note the release/commit so it clear when it was added.

[LeviPesin]

I think only the following entries were added there after r130:

• Material.vertexTangents
• ParametricGeometry
• TextGeometry
• FontLoader
• Font
• ImmediateRenderObject
• WebGLRenderer.gammaFactor
• WebGLMultisampleRenderTarget
• Euler.toVector3
• DataTexture2DArray
• DataTexture3D

[Mugen87]

@mrdoob After merging #24034, Object.defineProperties() is only used for two properties now (vertexTangents and gammaFactor). Do you think we can handle them (and upcoming legacy properties) differently such that Tree-shaking is not affected?

Would it be an option to move legacy properties and methods to classes and only keep stuff like export function Font... in THREE.Legacy.js? Now that the project has established a clear policy in context of removing legacy code, it should be more manageable to keep legacy properties and methods at their original spot.

[donmccurdy]

instanceof is even worse for tree-shaking.

Hm, but why?

With foo.prototype.isFoo = true the class can safely be tree-shaken, build tools just aren't perfect at detecting it. But if WebGLRenderer checked material instanceof PointsMaterial instead of material.isPointsMaterial, we could never tree-shake PointsMaterial because the class is now actually being imported and used.

We publish on NPM and it's not possible to do that without using semver, so we're already using it. We just do so wrongly...

Aside — We are and we aren't... Semver has a special case for versions <1.0.0, where breaking changes are allowed to occur in minor (0.x.0) releases. We're working within that special case. They presumably intended its use for "initial development", whereas we use it for other reasons, but tools like npm and yarn do understand that 0.141.0 → 0.142.0 is a potentially-breaking update.

[mrdoob]

@Mugen87

@mrdoob After merging #24034, Object.defineProperties() is only used for two properties now (vertexTangents and gammaFactor). Do you think we can handle them (and upcoming legacy properties) differently such that Tree-shaking is not affected?

Would it be an option to move legacy properties and methods to classes and only keep stuff like export function Font... in THREE.Legacy.js? Now that the project has established a clear policy in context of removing legacy code, it should be more manageable to keep legacy properties and methods at their original spot.

That sounds good to me 👍

We'll need to come up with a way to keep track of these legacy properties/methods though... 🤔

[LeviPesin]

Possibly just mark them with a @deprecated comment?

[Mugen87]

#24034 and #24040 should solve the original issue.

Let's not change prototype.is* for now and hope that bundlers handle this use case better in the future. If not, we can still implement the this.is* suggestion.

[LeviPesin]

AFAIK the only reason for choosing *.prototype.is* instead of this.is* was in decreasing the memory usage (so that there would be a boolean assigned not to every instance of class, but only one boolean assigned to the class prototype). Is it really an issue?

I think there are already used things like this.type in Material, which assign strings to every instance instead of prototype (BTW, I think they can be also changed to prototype assigning?).

[pschroen]

I would also second the ES field declarations suggested by Rich Harris, browser support is good now and can still be transpiled if needed, but that would be for a different issue and a larger discussion across the entire library.

Thanks guys for the work on this one, we're getting there! 😉

[donmccurdy]

Looks good 👍

https://caniuse.com/?search=class%20fields

[marcofugaro]

Let's not change prototype.is* for now and hope that bundlers handle this use case better in the future. If not, we can still implement the this.is* suggestion.

@Mugen87 bundlers have difficulty handling this case, it will probably not get better in the future.
Filed #24047, let's discuss over there!

[pschroen]

I agree with @marcofugaro, I didn't say anything earlier because I'm feeling like I've stirred the pot enough on this topic, but really, we can't have the maintainers of all bundlers change the way their bundlers have been working this whole time, so three.js is tree-shakeable.

The bundlers are actually working fine, and work as expected when tree-shaking a flat bundle, or pure ES modules. I am happy we've made improvements with Rollup though. 😄

[lumebot]

Pure ES modules are the only way to avoid importing the entire library for people using native ES modules in their app with no build tool and hence no tree shaking. Someone who wants to import math classes directly in Node.js, or with native ESM in a browser doesn't want to import the whole library.

Another issue is that if the library publishes both a bundle, as well as a src/ file, to NPM, then some people will import one or the other, and eventually an app will be bigger from duplicate classes, plus have the opportunity for bugs. Ideally only the bundle, or only src/, would be published to NPM, for consistency.

Pure ES modules would be the best way to go.

(prior discussion about pure ES modules)

[donmccurdy]

We will not be changing the NPM entrypoint to source files without a bundling step. I don't believe referring to that option as "pure ES modules" is particularly meaningful. This has been discussed at length before, as you know.

[pschroen]

Fair enough, whatever you want to call it, importing from the source files is side-effect free and works with tree-shaking, compared to the bundle which still has side-effects even after r141.

Though all the changes have made a huge difference in my test bundle, reducing the total size down to only 295 KB now (uncompressed), a savings of ~71%!

The goal with tree-shaking here is I should be able to import a class from the bundle, and only get the classes needed. In my test bundle importing only Vector2 is producing the 295 KB file.

We also have agadoo as a way to test. I'm happy to help with troubleshooting the remaining side-effects in the bundle if you guys like with a new issue? 😊

[mrdoob]

@pschroen Yeah, would be good to see what the next steps would be 👍