Making 'three' tree-shakeable

pschroen commented 2 years ago

The goal with tree-shaking here is I should be able to import a class from 'three', and only get the classes needed. In my test bundle importing only Vector2 is producing a 295 KB (uncompressed) file still with lots of remaining side-effects even after r141 and all the work done on #24006 (down from a 1 MB bundle in r140).

I'm opening this issue to resolve the remaining side-effects, which is do-able with some more work, and we have a couple ways of testing that.

Also to make the claim that 'three' is finally fully tree-shakeable, we'll need to verify that in the most popular bundlers. I'll start with Rollup, Webpack, Parcel and esbuild. Open to suggestions of other bundlers and configurations, and will start with a simple test of importing only Vector2.

Steps to reproduce the behavior:

import { Vector2 } from 'three';

console.log(new Vector2(0, 1));

And with agadoo:

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

It's worth noting that importing from the source files with agadoo also fails, and is something I can look into as well.

npx agadoo src/Three.js
...
Failed to tree-shake src/Three.js

The expected behavior, regardless of agadoo, is simply looking at the output bundle. If I import Vector2, I'm expecting only the @license header comment, and Vector2, nothing else.

References:

https://twitter.com/MarcoGomezGT/status/1520866649485754372
mrdoob/three.js#24006

marcofugaro commented 2 years ago

Other related discussions: #21667 #21665

pschroen commented 2 years ago

@mrdoob @Mugen87 @donmccurdy @LeviPesin I'd also like to use this issue to discuss the use of instanceof, does anyone have an example of it not working?

My fear here is that something has been misunderstood with how tree-shaking works and has led us all on a wild goose chase. Would be great to get some insight from either @Rich-Harris or @lukastaegert on this? See https://github.com/mrdoob/three.js/issues/24006#issuecomment-1120764103.

In my experience it's always been working for me, in Rollup at-least (and I've been using it since 2016). For example, I've been using my own TextureLoader, and when bundling my own classes in with other libraries the bundler should be renaming conflicting object names so when they are instantiated instanceof will always be comparing the correct constructor class:

TextureLoader becomes TextureLoader$1.
So the actual code output in the bundle would be loader instanceof TextureLoader$1, which works.
Code minification also works so long as the object names all match.

If there is an issue here, in my opinion that would actually be an issue with the bundler or minification, not with the use of instanceof.

Related discussion: #24167

LeviPesin commented 2 years ago

Does instanceof work with tree-shaking? I.e. if, for example, WebGLRenderer has .isPointsMaterial check in its body but does not have PointsMaterial import, does the changing it to instanceof PointsMaterial and importing PointsMaterial will break tree-shaking or not? I.e. will the PointsMaterial class be included? Related: https://github.com/mrdoob/three.js/issues/24006#issuecomment-1121315138

pschroen commented 2 years ago

Oh I think I get why this might be an issue, is the problem simply that PointsMaterial would be included in the bundle?

Technically it is being used in that case, and would be included in the bundle. If the goal is to not include any of the materials with WebGLRenderer, then ya we would have to use .isPointsMaterial.

I'll add this to my list of things to investigate...

LeviPesin commented 2 years ago

Oh I think I get why this might be an issue, is the problem simply that PointsMaterial would be included in the bundle?

Yes.

If the goal is to not include any of the materials with WebGLRenderer, then ya we would have to use .isPointsMaterial.

I think we should calculate how often such pattern is used (i.e. the class is not imported, but .isClass is used)...

marcofugaro commented 2 years ago

I.e. if, for example, WebGLRenderer has .isPointsMaterial check in its body but does not have PointsMaterial import, does the changing it to instanceof PointsMaterial and importing PointsMaterial will break tree-shaking or not? I.e. will the PointsMaterial class be included?

Yup, it will be included. This is the reason three switched to .is* in #9310.

LeviPesin commented 2 years ago

And there is a more detailed explanation in https://github.com/mrdoob/three.js/pull/9310#issuecomment-232004849.

BTW (completely unrelated to this issue but related to that), is the reasons for not using default exports valid now? They are very useful to avoid constantly writing import { Abc } and export { Abc } (they are already used in e.g. the nodes system).

marcofugaro commented 2 years ago

BTW (completely unrelated to this issue but related to that), is the reasons for not using default exports valid now? They are very useful to avoid constantly writing import { Abc } and export { Abc } (they are already used in e.g. the nodes system).

Code style I believe.

LeviPesin commented 2 years ago

And there is a more detailed explanation in https://github.com/mrdoob/three.js/pull/9310#issuecomment-232004849.

In that comment the following link was given: https://github.com/mrdoob/three.js/issues/4776#issuecomment-71575905 (It is said there that polymorphism can be used instead of .is checks) I think we can use the same pattern in most such occurences?

pschroen commented 2 years ago

is the reasons for not using default exports valid now?

Code style I believe.

Agree with @marcofugaro, it's an opinionated topic as a result.

Using named exports is supposed to be friendlier for tree-shaking when you are always explicitly using the named imports, but in practice I've found it makes no difference in my experience, the bundler renames all the objects anyways, and I use both default exports and named exports, though over the years I've moved more towards named exports/imports for consistency and being more explicit.

LeviPesin commented 2 years ago

I think we can use the same pattern in most such occurences?

E.g. the first occurence of .is that GitHub search shows - https://github.com/mrdoob/three.js/blob/dev/src/renderers/shaders/UniformsUtils.js - can be easily fixed by changing the value.isVector2 || ... to value.clone !== undefined. Will file a quick PR for this.

donmccurdy commented 2 years ago

Sorry I don't see how changing obj.isPointsMaterial to obj instanceof PointsMaterial could ever benefit tree-shaking. Using instanceof will certainly create a reference to the class and prevent tree-shaking, and .is does not...

LeviPesin commented 2 years ago

let's not remove .is properties generally please. See https://github.com/mrdoob/three.js/issues/24199#issuecomment-1149000155

My suggestion is not to replace .is with not-tree-shakable instanceof - my suggestion is to replace it with polymorphism and duck typing, like in https://github.com/mrdoob/three.js/issues/4776#issuecomment-71575905 and https://github.com/mrdoob/three.js/pull/24202.

(I should note that I am not talking about duck typing classes via unrelated methods, I am talking about duck typing them with only the methods used in the code block - like in https://github.com/mrdoob/three.js/pull/24202)

pschroen commented 2 years ago

@donmccurdy Yes agreed, there's been some confusion in the terminology, when I read that instanceof doesn't work with tree-shaking I was taking it literally, technically it does work, it just works against this library because of the way the types are being checked.

I think there's some good discussion here on alternative approaches as above. I'm going to focus more on removing the remaining side-effects in the meantime.

donmccurdy commented 2 years ago

My suggestion is not to replace .is with not-tree-shakable instanceof - my suggestion is to replace it with polymorphism and duck typing

But why? I guess I don't see any benefit to this — just a lot of work, harder-to-read code, and a hard-to-predict chance of causing classic performance pitfalls associated with polymorphism. If there is some benefit, let's discuss it in another thread since it doesn't seem to be related to this one?

pschroen commented 2 years ago

Agreed, even though related, the bundle being tree-shakeable in its current state doesn't hinge on the type check approach. 👍

pschroen commented 2 years ago

Alright, so I've spent some more time tonight with the latest, here's a summary of my findings. 😉

src/Three.js itself has side-effects at the bottom because of the checks for __THREE_DEVTOOLS__ and multiple instances. #24225
Three.Legacy.js is fully tree-shakable now, nice work guys! 🎉 #24006
The DataUtils.js file has a number of expressions and for loops causing side-effects. #24218
The PropertyBinding.js, KeyframeTrack.js, Materials.js, Material.js, Object3D.js, Euler.js, Texture.js and Color.js files have static and prototype properties that could be converted to class fields. Have we made a decision on the use of class fields?
Material.js has a side-effect at the bottom with Material.fromType and a // TODO: Behavior added in Materials.js, can that be removed? #24094
ShaderLib.js, UniformsLib.js and ShaderChunk.js are also side-effects, perhaps there's a different approach we could take with defining these shaders? #24221
ObjectLoader.js creates a big side-effect with the Geometries namespace, removing that one line allows for tree-shaking.
WebGLRenderer.js of-course is loading a lot of classes including the shader classes with side-effects, which in-turn creates a domino effect importing all the classes from ShaderLib.js and UniformsLib.js, etc., removing those classes allows for tree-shaking as well.
And finally MathUtils.js has a side-effect at the top with the _lut for loop. #24210

Removing all of the above, shader and renderer classes from src/Three.js is allowing me to build a module bundle with npm run build-module and then in-turn import from build/three.module.js side-effect free.

npx agadoo

Success! build/three.module.js is fully tree-shakeable

Still lots of work to do but we're getting there! 😅

lukastaegert commented 2 years ago

So while relying on duck typing alone is probably best for tree-shaking, instanceof is an improvement as it allows for some basic static analysis. I created rollup/rollup#4524 for you, you can try out if this would help.

mrdoob commented 2 years ago

@lukastaegert Nice!

Maybe we can try using instanceof with Vector2, Vector3 and Vector4. That should solve #24167.

@pschroen Would you like to give it a go?

pschroen commented 2 years ago

Sure, though I feel like it might be a bigger can of worms for consistency? I am a fan of a hybrid approach for type checks, so maybe .is properties for classes used by WebGLRenderer to keep the bundle size down, and then instanceof for the math classes? Is there any performance impact with instanceof compared to .is?

For reference I was looking at this file first. https://github.com/mrdoob/three.js/blob/de17358232c3cc45bb2df0a5a64068cbcdeaa2f8/examples/jsm/nodes/core/NodeUtils.js#L33

mrdoob commented 2 years ago

Is there any performance impact with instanceof compared to .is?

I want to think that instanceof is faster... 🤞

LeviPesin commented 2 years ago

So now (after merging that PR, actually) when Rollup tree-shakes instanceof, we can convert all of the .is checks to instanceof?

marcofugaro commented 2 years ago

when Rollup tree-shakes instanceof, we can convert all of the .is checks to instanceof?

No, this would make the whole repo un-treeshakeable again 😅

Rollup isn't the only bundler out there.

LeviPesin commented 2 years ago

I think converting some .is* to instanceof and not converting others would only create a mess.

Maybe we should create an issue for tree-shaking instanceof in other popular bundlers repositories? Or even a PR based on https://github.com/rollup/rollup/pull/4524.

marcofugaro commented 2 years ago

Maybe we should create an issue for tree-shaking instanceof in other popular bundlers repositories? Or even a PR based on https://github.com/rollup/rollup/pull/4524.

Good luck with that, we tried in the past with tree-shaking of .prototype. and it was never supported.

pschroen commented 2 years ago

Ya agreed, support for Rollup has always been amazing (thanks @lukastaegert ❤️), we can do so much more with tree-shaking in Rollup now, though the goal of this issue is to get tree-shaking working for all major bundlers. 🥲

I do agree with @donmccurdy this should also be a separate issue and discussion now at this point, and I'm on the fence about the consistency of the change, I do like the idea of a hybrid approach with the math classes, however it does also add a level of inconsistency.

Ultimately it's up to you guys, should we try a hybrid approach in a new issue, or stick with the .is* type checks for now?

LeviPesin commented 2 years ago

Material.js has a side-effect at the bottom with Material.fromType and a // TODO: Behavior added in Materials.js, can that be removed?

See https://github.com/mrdoob/three.js/pull/24094.

pschroen commented 2 years ago

A note on #24218 above,

The advantage of using singleton patterns in general is it reduces the expressions evaluated by the browser at runtime, loading resources only when needed.

Many of these side-effect expressions aren't even being used, but the browser is executing this code regardless, which is why they are side-effects.

Ideally for a library to be "pure" there should be no code running at all outside of the classes. To quote Rich Harris' agadoo again:

Don't create instances of things on initial evaluation — instantiate lazily, when the functions you export are called

I'd imagine this would improve load time, and also reduce memory usage a little as well. 😉

pschroen commented 2 years ago

Referencing @marcofugaro's https://github.com/mrdoob/three.js/pull/24218#issuecomment-1151224477,

Sure, I could convert that to plain functions like in MathUtils, that's also one of the recommended approaches from agadoo:

Export plain functions

The only catch, similar to MathUtils's _lut is there's code being evaluated to generate the lookup tables. I suppose I could convert that to an internal _generateTables() function which should work with tree-shaking too?

pschroen commented 2 years ago

@marcofugaro Is there any reason to keep the DataUtils namespace in this case? In other words, is anyone externally using this class, it's in the docs:

https://threejs.org/docs/#api/en/extras/DataUtils

marcofugaro commented 2 years ago

The only catch, similar to MathUtils's _lut is there's code being evaluated to generate the lookup tables. I suppose I could convert that to an internal _generateTables() function which should work with tree-shaking too?

You can use the /*@__PURE__*/ annotation for function calls as well, so for _lut it would be

const _lut = /*@__PURE__*/ generateLut()

Let me know if you need any help. Full docs are here.

marcofugaro commented 2 years ago

In other words, is anyone externally using this class, it's in the docs:

Yes I believe people are using that module. The way they import would change, like it did for BufferGeometryUtils: https://github.com/mrdoob/three.js/pull/22267

marcofugaro commented 2 years ago

Btw you can test tree-shaking by running npm run test-treeshake

pschroen commented 2 years ago

Have we made a decision on the use of class fields?

Following-up on this one, I know we've already made a lot of progress since r140 with removing .prototype.* properties, though we still have quite a few static properties that could be converted to class fields. See https://github.com/mrdoob/three.js/issues/24006#issuecomment-1123779387

What's the verdict on this one?

LeviPesin commented 2 years ago

I think we don't yet use class fields, optional chaining, and nullish coalescing in the core. It's maybe a time to change that?

pschroen commented 2 years ago

Alright, so @marcofugaro I've created a draft PR above for discussion on using static class fields.

We moved away from the "class with only static methods" pattern

Note that adding the pure annotations to ShaderLib and UniformsLib does appear to be working in Rollup with the original object literal as well, so really the only advantage with the static class fields is having the physical property inside the class which works with this.

Also some great news, because it appears that those two files were the linchpin for tree-shaking the entire flat bundle with the latest version of Rollup! 🎉

ShaderLib and UniformsLib were creating a cascading side-effect across everything that touches them, the shader and renderer classes appear to be fine now, and simply adding the pure annotations to those two files allows for tree-shaking the entire flat bundle (with the exception of src/Three.js itself, but I'm saving my comments on that until everything else is done). 😊

LeviPesin commented 2 years ago

I actually think in this case it is better to have just an object with properties (and add physical there, obviously) rather than a class...

pschroen commented 2 years ago

@LeviPesin Sure, I'll push a new commit to the draft switching back to the original object literal, and then we can revisit class fields at a later time if needed. 👌

pschroen commented 2 years ago

Btw you can test tree-shaking by running npm run test-treeshake

@marcofugaro I finally got this command to work by downgrading chalk to v4.1.2, must be something wrong with the v5 ESM version. I'm running the latest version of Node v18.3.0.

Not sure about this as a tree-shaking test though, I would much prefer agadoo if we can get it working (it's currently broken as well because it's pre-Rollup v2).

Investigating a solution for either or, ideally we'd want something that will error-out we can add to three's test script. It's super easy to accidentally add side-effects in a library and think a goal of this issue should also be a success/fail unit test. 😉

pschroen commented 2 years ago

I should also clarify something here:

tree-shaking the entire library

I've edited that to be "flat bundle" instead, really this is tree-shaking a single class without side-effects at the highest level (which is why I chose Vector2 to start).

But there are plenty of deeper level tree-shaking issues, I would consider those secondary issues we can also solve with real world examples. A simple practical example is also easy to write a unit test for, for example:

import { BoxGeometry } from 'three';

console.log(new BoxGeometry());

import { MeshBasicMaterial } from 'three';

console.log(new MeshBasicMaterial());

import { MathUtils } from 'three';

console.log(MathUtils.clamp(0, 0, 1));

import { clamp } from 'three';

console.log(clamp(0, 0, 1));

I'll create separate issues for these, though for this issue I would consider tree-shaking Vector2 and a high-level unit test like agadoo enough to close this particular issue.

One last note on the terminology of "tree-shaking" that has been confusing a lot of people, and why I don't view tree-shaking as dead code removal, it's really the opposite, live code inclusion.

So I'm more thinking about what it's including, it's the opposite approach, like I shake this tree here and out drops only Vector2. 😉

pschroen commented 2 years ago

I've created a draft PR above for discussion on the registration event and multiple instances check, and if it's really needed on initial evaluation, or if it's fine when WebGLRenderer is instantiated? How would this work with the other renderers as well?

pschroen commented 2 years ago

Referencing @gkjohnson's https://github.com/mrdoob/three.js/pull/24225#issuecomment-1153069609 cc @Mugen87

Hmm, good points on both the "one library, multiple renderers" and "two libraries, one renderer". This is the reason why I was leaving this conversation till the end of troubleshooting this issue (for Rollup anyways).

For the second point, "two libraries, one renderer", this really conflicts with tree-shaking in general which needs a pure scope with nothing on initial evaluation. If the only way "two libraries, one renderer" can be checked is with something in global scope, on initial evaluation, then unfortunately this goes back to my use case points on how people use the library. See https://github.com/mrdoob/three.js/issues/24006#issuecomment-1120203227 😜

Let me revise it then, so in my opinion there are three use cases:

A "THREE namespace, script", like in the manual or when loading the pre-bundled library from a CDN, the multiple instances check makes sense here.
A "THREE namespace, ES module import", like in the docs or examples, the multiple instances check makes sense here too, though I feel like the onus is more on the application developer with this one.
An "ES module library", so I'm potentially not using a renderer and importing utility type classes, this is also the use case where more and more people are importing directly from the source files to get around the tree-shaking problems.

So given that, we could solve this particular problem if we were exporting the source files as the library entry point, keeping the bundles, you could just move the registration event and multiple instances check to the Rollup builds, appended to the bottom of the bundles, and then for people importing from the source files it wouldn't be a problem for them.

So unless we can think of an alternative approach, I think we're stuck in a little bit of a "catch-22" type of situation here, to fully tree-shake this library we'll need to remove that functionality. I realize it's there for convenience for new developers, but really I would argue loading two instances of the library, or accidentally bundling two namespaces (double the bundle size) isn't the responsibility of a library, the onus is on the application developer.

You could make this argument with any library. Perhaps we replace it with a "best practices" kind of thing in the docs or manual on how developers can avoid these accidental situations? This feels more like a developer problem imho. 😉

gkjohnson commented 2 years ago

For the second point, "two libraries, one renderer", this really conflicts with tree-shaking in general which needs a pure scope with nothing on initial evaluation.

I haven't been following the full thread abut where has it been shown that these lines need to be removed in order to tree shake the full library?

That aside I think there are still improvements that can be made to that section of code:

I think the __THREE_DEVTOOLS__ event dispatch can be safely moved to the WebGLRenderer initialization as long as it's only run once which means we only have to worry about the multiple imports check.
As you suggest move the multiple instances checks to just the bundled code which will cover the new-user use cases and catch issues there. And if you want advanced, full-tree-shaking behavior you can configure your bundler to pull from "src/Three.js"

really I would argue loading two instances of the library, or accidentally bundling two namespaces (double the bundle size) isn't the responsibility of a library, the onus is on the application developer

Unfortunately we're still seeing cases where people are running into the "multiple instances of three" problem on the forum so I don't think it should go if we can avoid it - it's a really difficult problem to debug without with warning.

LeviPesin commented 2 years ago

As you suggest move the multiple instances checks to just the bundled code

I support this idea.

pschroen commented 2 years ago

Sounds good, I'll update the draft moving them to the bundled code.

@gkjohnson Ya these are causing side-effects, adding pure annotations to these also makes no difference with tree-shaking because they are global scope. I'll think on other possible ways around this as well...

pschroen commented 2 years ago

Alright, so this is the point where we hit the dead-end of "fully" tree-shaking 'three', we've made huge improvements over the past month though, and there's still much to do with the secondary side-effects. See https://github.com/mrdoob/three.js/issues/24199#issuecomment-1152913575

This is as far as we can take Rollup, even with all of @lukastaegert's latest updates. See #24225

git clone --depth=1 https://github.com/mrdoob/three.js.git
cd three.js 
npx pschroen/agadoo

/**
 * @license
 * Copyright 2010-2022 Three.js Authors
 * SPDX-License-Identifier: MIT
 */
const REVISION = '142dev';

if ( typeof __THREE_DEVTOOLS__ !== 'undefined' ) {

    __THREE_DEVTOOLS__.dispatchEvent( new CustomEvent( 'register', { detail: {
        revision: REVISION,
    } } ) );

}

if ( typeof window !== 'undefined' ) {

    if ( window.__THREE__ ) {

        console.warn( 'WARNING: Multiple instances of Three.js being imported.' );

    } else {

        window.__THREE__ = REVISION;

    }

}

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

There's a few directions we can go from here, how do you guys feel about exploring a separate package for three's source files?

From @donmccurdy:

I'd be perfectly happy to see src/ removed from the npm package. That's mostly rhetorical -- I'm not seeing any real reason to delete at this time -- but certainly I don't think we should be paving paths to support imports from src/, this is not an intended usage pattern.

I do agree, though it is actively being used by a lot of people to get around these tree-shaking problems, and I also prefer working directly with the source files.

It's also directly causing the problem we're having with multiple namespaces being imported and the need for a multiple instances check for applications using ES modules.

In other words, I am in support of removing src/ from the npm package, so long as we give users another way to access the source files via npm.

@marcofugaro Are there other cases of multiple namespaces that you know of? In my experience all of the multiple namespace problems have been this exact issue?

Mugen87 commented 2 years ago

I'd be perfectly happy to see src/ removed from the npm package.

Yes, that sounds reasonable. I would also prefer to just have single way how classes from the core can be imported 👍 .

However, I do not vote for having more than one three npm package. That would make the rollout and maintenance just more complicated.

I'd also like to raise the question if the project needs to be perfectly tree-shakable. 100% solutions are often tricky to implement and the added value not always justifies the efforts. Maybe we can just state that the current result is "good enough" and focus on other topics?

donmccurdy commented 2 years ago

In other words, I am in support of removing src/ from the npm package, so long as we give users another way to access the source files via npm.

I don't think it is a good idea to mix npm and grabbing src/ files, or to have multiple versions of the library on npm. If users prefer to manage dependencies manually, without NPM, they can do so quickly:

npx degit mrdoob/three.js/src#r141  ./three-src

Still, this is not a workflow I would encourage anyone to use unless you're very sure of what you're doing and its implications. Personally I've stopped responding to forum questions questions related to project setup, unless the author is using npm and a bundler. Providing so much support for non-standard installation methods is an endless waste of everyone's time.

pschroen commented 2 years ago

@Mugen87 I hear ya, maybe we just leave this as is then for now, as @gkjohnson put it above:

And if you want advanced, full-tree-shaking behavior you can configure your bundler to pull from "src/Three.js"

That's what started this whole conversation with @MarcoGomezGT's tweet on the topic. He's not the only one doing this, most developers I'm working with these days are all working this way already, myself included.

I don't normally stir the pot on these things though after years of dealing with this same problem myself I wanted to clear-up some confusion on it, and wanted to give it a go myself. I'm pretty happy with the results we've achieved in Rollup, but there's more that can be done with the other bundlers.

gkjohnson commented 2 years ago

I'd also like to raise the question if the project needs to be perfectly tree-shakable. 100% solutions are often tricky to implement and the added value not always justifies the efforts. Maybe we can just state that the current result is "good enough" and focus on other topics?

I agree - it would be nice to quantify the effect of the multiple instances check a bit more, though. How much is it impacting the tree shake-ability?

Assuming it's significant I think there may still be some alternatives to consider before giving up on tree shaking - first as I mentioned in I think the __THREE_DEVTOOLS__ can safely be moved to the WebGLRenderer constructor without any loss in functionality which leaves us with just the multiple instances check.

For the multiple instances check I know at least webpack has the ability to prune code encapsulated in constant conditions, such as when building for production (it's an option, at least):

if ( typeof process !== 'undefined' && process.env.NODE_ENV !== 'production' ) {

    // this code will be pruned in production builds

}

Maybe we can add a define flag for three or use an existing production flag to enable that multiple instances check to be pruned only in prod builds? Do we know if there's a standard for this kind of thing across bundlers?

mrdoob / three.js

Making 'three' tree-shakeable #24199