Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nodes: Mark as effectful for Webpack. #27189

Merged
merged 1 commit into from Nov 15, 2023

Conversation

CodyJasonBennett
Copy link
Contributor

Fixes: #27076

Description

Configures sideEffects to exclude three/nodes from coarse module skipping prior to tree-shaking. Since #26881, Nodes are effectful and cannot be individually tree-shaken.

Copy link

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
667.4 kB (165.9 kB) 667.4 kB (165.9 kB) +0 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
449.3 kB (108.8 kB) 449.3 kB (108.8 kB) +0 B

@Mugen87 Mugen87 added this to the r159 milestone Nov 15, 2023
@mrdoob mrdoob merged commit 1bd7fcf into mrdoob:dev Nov 15, 2023
12 checks passed
@CodyJasonBennett CodyJasonBennett deleted the nodes-sideeffects branch November 15, 2023 10:29
@emnul
Copy link

emnul commented Nov 17, 2023

Thanks for the fix! I was running into issues with my vite build due to this.

@hybridherbst
Copy link
Contributor

Out of curiosity, was it even intentional that #26881 now requires these additional steps? I can't see a mention of it in the original PR so can only assume that was by accident. I think it would be better if there's no additional configuration needed.

@mrdoob
Copy link
Owner

mrdoob commented Dec 22, 2023

Out of curiosity, was it even intentional that #26881 now requires these additional steps? I can't see a mention of it in the original PR so can only assume that was by accident. I think it would be better if there's no additional configuration needed.

/ping @sunag

@hybridherbst
Copy link
Contributor

Seems vite ignores both exports and sideEffects in locally referenced dependencies:

@LeviPesin
Copy link
Contributor

Out of curiosity, was it even intentional that https://github.com/mrdoob/three.js/pull/26881 now requires these additional steps? I can't see a mention of it in the original PR so can only assume that was by accident. I think it would be better if there's no additional configuration needed.

I'm pretty sure Nodes became effectful since at least the introduction of node chaining (I've explicitly noted that tree-shaking wouldn't work now in #25526). I've also suggested a possible workaround in that issue.

AdaRoseCannon pushed a commit to AdaRoseCannon/three.js that referenced this pull request Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(r158 nodes) Webpack error in ModelNode
6 participants