Making 'three' tree-shakeable #24199
Comments
|
@mrdoob @Mugen87 @donmccurdy @LeviPesin I'd also like to use this issue to discuss the use of 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 #24006 (comment). 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
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 Related discussion: #24167 |
|
Does |
|
Oh I think I get why this might be an issue, is the problem simply that 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 I'll add this to my list of things to investigate... |
Yes.
I think we should calculate how often such pattern is used (i.e. the class is not imported, but |
Yup, it will be included. This is the reason three switched to |
|
And there is a more detailed explanation in #9310 (comment). 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 |
Code style I believe. |
In that comment the following link was given: |
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. |
E.g. the first occurence of |
|
Sorry I don't see how changing |
My suggestion is not to replace (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 #24202) |
|
@donmccurdy Yes agreed, there's been some confusion in the terminology, when I read that 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. |
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? |
|
Agreed, even though related, the bundle being tree-shakeable in its current state doesn't hinge on the type check approach. 👍 |
|
Alright, so I've spent some more time tonight with the latest, here's a summary of my findings. 😉
Removing all of the above, shader and renderer classes from npx agadoo
Success! build/three.module.js is fully tree-shakeableStill lots of work to do but we're getting there! 😅 |
|
So while relying on duck typing alone is probably best for tree-shaking, |
|
@lukastaegert Nice! Maybe we can try using @pschroen Would you like to give it a go? |
|
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 For reference I was looking at this file first. |
I want to think that |
|
So now (after merging that PR, actually) when Rollup tree-shakes |
No, this would make the whole repo un-treeshakeable again 😅 Rollup isn't the only bundler out there. |
|
I think converting some Maybe we should create an issue for tree-shaking |
Good luck with that, we tried in the past with tree-shaking of |
The existing code style is not bad per se. Since the There is also the risk of introducing bugs when migrating code. When there is no other reason than improving the code style, I would not vote to touch the renderer's code base. |
|
ES6 classes are just cleaner and easier to read. |
|
Hey @marcofugaro @Mugen87 @LeviPesin 👋,
All the
Hmm, I see, ya @Mugen87 you're right it's primarily code style and syntactic sugar, though there is one small case with This is a can of worms question, are there any other ways of defining a WebGL 1 rendering context, perhaps by passing-in a parameter to the constructor? |
You can pass in a custom rendering context to |
|
Ya I'm not sure if deprecating the |
|
No, I don't think so^^. |
|
As an update on tree-shaking the shaders, I've spent some time yesterday and today trying to make them fully tree-shakeable, I've almost got it working, but hit a couple blockers preventing me from finishing the work.
How would you guys feel about possibly opening-up a use case where the |
|
Ya, @marcofugaro I've hit the same dead-end with And the savings from Well, I gave it my best shot guys, and it's been a good learning experience being knee-deep in the three.js renderer and shaders. There's not much we can do about the remaining static and
After #24336 is merged I think we can close this issue now, we've gone as far as we can with the shaders and webpack for now. 🫠 |
|
Is there a summary of the issue with import { x } from 'path/to/chunks/x.glsl.js';
import { y } from 'path/to/chunks/y.glsl.js';
class MeshStandardMaterial extends Material { ... }
MeshStandardMaterial.ShaderChunk = { x, y };
export { MeshStandardMaterial };Still a breaking change, but manageable. Or have I missed the issue? Another consideration is that the node-based material system is still in development, and will be the primary system for WebGPU, I assume. I would guess its interaction with tree-shaking is quite different, and probably more flexible. |
|
You got it, that would be the next step I was going to take but feeling it's not really worth it. See #21665 (comment) Writing custom shaders with three.js is already a little awkward, and I think this would make it even more complicated. It's more convenient having all the chunks in one place I think. 🙃 |
My impression is that there are a few issues that could ultimately affect the end user if not handled properly. Take this all with a grain of salt but this is what I've gathered over from roughly following these conversations:
Point 2 is what makes this hard because it means we can't just remove the
If I'm understanding all this tree shaking stuff then that should afford the ability for only the required shader chunks and uniform sets to be included in a tree-shaken bundle. The built-in material code might look like this: import { bsdfs, uv_pars_fragment, ... } from 'path/to/chunks/ShaderChunk.js';
import { ShaderChunkLib } from 'path/to/chunks/ShaderChunkLib.js';
class MeshStandardMaterial extends Material {
constructor() {
super();
ShaderChunkLib.register( 'bsdfs', bsdfs );
ShaderChunkLib.register( 'uv_pars_fragment', uv_pars_fragment );
// ...
}
}
// ... migration function for end users
import { registerAllShaderChunks } from 'three';
registerAllShaderChunks();
// all built-in chunks available for user-written shadersAdmittedly the above also looks like a huge pain to maintain. But perhaps there's a more svelte pattern for this (like what @donmccurdy suggested). Though at the least it seems like something like this would get things working without dramatically impacting end users. |
|
Interesting, ya something that's evaluated at runtime would work much better with tree-shaking, to quote agadoo again:
|
|
One more related discussion: |
|
As an update on agadoo, Rich Harris has updated it to the latest version of Rollup with the release of git clone --depth=1 https://github.com/mrdoob/three.js.git
cd three.js
npx agadoo |
|
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;
}
}Then run |
|
It seems the The multiple instance check is helpful in certain cases but I wonder if it's more important than full tree shake support. Granted, it's a bit of a hack that the library writes to |
Personally I think the multi-import check is more helpful, yes, if everything except the 6-7 lines of code required are being tree-shaken. If dev tools were actively maintained I would vote to keep those lines too, but as it is... perhaps not. The difference between 99.9% tree-shaking and 100% tree-shaking is basically nothing. Or it may be possible to wrap those lines in a block like this: if (process?.env?.NODE_ENV === 'development') {
// ...
}Not sure about agadoo, but bundlers should strip that code except during local development. |
Nope, bundlers do a string replacement of the The multi-import check has been proven useful for me multiple times, so I vote we leave it. |
|
Probably possible to work around that? const process = typeof global === 'undefined' ? {env: {}} : global.process;
if (process.env.NODE_ENV === 'development') {
// ...
}But either way, I don't think having those few lines remaining is a problem. |
This is a good question, which of these is true?
If it's 1. then I don't see any problem with these lines remaining. |
In my experience, that doesn't happen. |
|
Should we close this then? |
|
I would say yes. Related topics like the usage of class properties can be discussed in specific PRs or issues. |
|
Agreed, you want to do the honours, or should I close it? 😅 |
|
🎉 |
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 onlyVector2is producing a 295 KB (uncompressed) file still with lots of remaining side-effects even afterr141and all the work done on #24006 (down from a 1 MB bundle inr140).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 onlyVector2.Steps to reproduce the behavior:
And with agadoo:
It's worth noting that importing from the source files with
agadooalso fails, and is something I can look into as well.The expected behavior, regardless of
agadoo, is simply looking at the output bundle. If I importVector2, I'm expecting only the@licenseheader comment, andVector2, nothing else.References:
The text was updated successfully, but these errors were encountered: