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

refactor: improve algorithm #10

Merged
merged 8 commits into from
Oct 21, 2023

Conversation

CodyJasonBennett
Copy link
Member

Rewrites the plugin and its test suite to handle member, dynamic, and template JSX expressions in addition to tree-shaking the default <Canvas />. This removes all of the limitations of the plugin, so I've opted to cleanup the README.

package.json Outdated Show resolved Hide resolved
@mklasinc
Copy link

Is there perhaps a timeline estimate with regards to merging this PR to main? I think it's an awesome improvement and I'd be keen to try it out in production! 🙌

@CodyJasonBennett
Copy link
Member Author

CodyJasonBennett commented Jun 19, 2023

You can copy the source and pass it to Babel if you want to try right now, but unfortunately I just left a position where I wanted to field test this. Just integration testing for different tools and the effectiveness of tree-shaking, don't see any other blockers.

@umar-ahmed
Copy link
Collaborator

Is there perhaps a timeline estimate with regards to merging this PR to main? I think it's an awesome improvement and I'd be keen to try it out in production! 🙌

Gonna try to review this week 🤞. I don't see major blockers. But this is a rewrite of the logic and I want to make sure it's not breaking any existing use-cases.

@stilettk
Copy link

Hello! Is there any time estimate for this? It would be so great to use Canvas out-of-the-box with all of its features, and with the tree-shaking support!

@CodyJasonBennett
Copy link
Member Author

I did a lot of plumbing work upstream with tree-shaking and can verify that works as intended with https://github.com/CodyJasonBennett/treeshake-cli. extend calls are not inlined for the moment.

@stilettk
Copy link

I'm taking about using Canvas, as mentioned here: #9 (comment). What you mean is that you have tested default Canvas with treeshake-cli, and tree-shaking works as expected? I'm asking because the screenshot shows only Vector import.

@CodyJasonBennett
Copy link
Member Author

CodyJasonBennett commented Oct 20, 2023

The previous blockers for this PR was whether it produces valid code after tree-shaking (mostly important for libraries which use this plugin) and is configurable between build tools. I've cleared both so it's just pending review from @umar-ahmed.

@umar-ahmed
Copy link
Collaborator

umar-ahmed commented Oct 20, 2023

Committing to reviewing this weekend. You have my permission to keep pinging me to get it to the finish line 😛

Copy link
Collaborator

@umar-ahmed umar-ahmed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CodyJasonBennett Looks good overall. I had a couple comments/questions. But I think we can get this merged and released soon.

We did lose one bit of functionality, which is being able to import components from packages other than three. I think it's fine if we omit this for now, but I just wanted to call it out for future readers.

README.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
src/index.test.ts Outdated Show resolved Hide resolved
@CodyJasonBennett
Copy link
Member Author

CodyJasonBennett commented Oct 21, 2023

We did lose one bit of functionality, which is being able to import components from packages other than three. I think it's fine if we omit this for now, but I just wanted to call it out for future readers.

I didn't understand this use-case or the need to begin with rather. This plugin assumes a well-formed program which means it must be portable with/without it.

CodyJasonBennett and others added 2 commits October 21, 2023 14:34
Co-authored-by: Umar Ahmed <umar.ahmed1998@gmail.com>
@umar-ahmed umar-ahmed merged commit be04652 into pmndrs:main Oct 21, 2023
1 check passed
@CodyJasonBennett CodyJasonBennett deleted the refactor/improve-algorithm branch October 21, 2023 20:51
@umar-ahmed
Copy link
Collaborator

Published to NPM and created a release on GitHub: https://github.com/pmndrs/react-three-babel/releases/tag/v1.0.0

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.

None yet

4 participants