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

fix(troika-three-text): remove IIFEs for tree-shaking #224

Merged
merged 6 commits into from Dec 15, 2022

Conversation

CodyJasonBennett
Copy link
Contributor

Fixes #215. IIFEs cannot be statically analyzed for tree-shaking -- their scope is forced into the bundle regardless of usage.

@lojjic
Copy link
Collaborator

lojjic commented Nov 14, 2022

🤔 IIRC, those IIFEs were actually necessary at one point for proper tree shaking; there was something in them (prototype mutation, perhaps?) which was confusing bundlers at the time and the IIFE with PURE comment worked around it. Maybe that's not a problem anymore, or it's just one specific bundler with the issue? (I think I was using Rollup but my memory's hazy.)

Assuming you've verified this fixes treeshaking with the drei stack, I'd be ok accepting the change. Worst case we find out it breaks treeshaking elsewhere and have to come up with another fix.

@CodyJasonBennett
Copy link
Contributor Author

CodyJasonBennett commented Nov 14, 2022

I can verify this on my end today/tomorrow. This should be released before my other PR so it can be backported. Pure comments are needed for all global side effects, but we've sorted this downstream if three was a factor.

@CodyJasonBennett
Copy link
Contributor Author

CodyJasonBennett commented Nov 15, 2022

There was indeed a prototype mutation for r109 compat. I've moved it to the class as an overload in b55a0c8. This or similar work-arounds <r125 can be removed following #215.

I've verified tree-shaking works with https://npmjs.com/agadoo. I will manually review with Rollup, ESBuild, Webpack, and Parcel. Are there any tools I'm missing? Happy to create a test repo for future reference or integration testing.

@CodyJasonBennett
Copy link
Contributor Author

I created a generic testing repo and a branch with a local copy of this PR at https://github.com/CodyJasonBennett/treeshaking/tree/troika. Tree-shaking seems to work everywhere but Parcel which doesn't seem to tree-shake with the given configuration. I tested between Rollup, ESBuild, Vite, Webpack, and Parcel.

@lojjic
Copy link
Collaborator

lojjic commented Dec 14, 2022

@CodyJasonBennett We've got some merge conflicts now, could you please resolve them? I've merged #225 so the workarounds you mentioned can probably be removed too.

Edit: nevermind about there being conflicts, I was misreading. We can still remove the workarounds though?

@lojjic
Copy link
Collaborator

lojjic commented Dec 14, 2022

Actually there may be some mis-resolved conflicts -- I'm seeing a return from PlaneGeometry to PlaneBufferGeometry in the diff...?

@CodyJasonBennett
Copy link
Contributor Author

Resolved them.

@lojjic lojjic merged commit 2e688f0 into protectwise:main Dec 15, 2022
@CodyJasonBennett CodyJasonBennett deleted the fix/three-tree-shaking branch December 15, 2022 04:02
@lojjic
Copy link
Collaborator

lojjic commented Dec 15, 2022

Thank you!

@lojjic
Copy link
Collaborator

lojjic commented Dec 15, 2022

v0.47.0 is published.

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.

Allow troika-three-text to work with bundle tree-shaking
2 participants