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: imports nodes/tags only when needed #145

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

tcmhoang
Copy link
Contributor

This is my initial attempt to address the tree-shaking issue. I've focused on minimizing changes in the codebase while ensuring functional parity. However, some adjustments need to be made in the test suite, I have not touched anything in that folder yet.

@TorstenDittmann
Copy link
Owner

Hehe, nice timing. I just finished my PoC :-D

I got something working too, however - the partials are super annoying. Because used nodes & tags are not part of the node AST. Means for each partial used, we need to walk down the tree recursively.

@TorstenDittmann
Copy link
Owner

TorstenDittmann commented Apr 12, 2024

I'll try and see to review this weekend 👍🏻

fyi, here is my poc #146 and I stopped when I realized the partials part.

@tcmhoang
Copy link
Contributor Author

Good news 🎉. I think I've solved the issue with partials. The demo website works great now.

I noticed the changes in the PR #146 , using the extended type Schema & {...} to support alias import (Not sure how to call it in Typescript though 🤔). While it's interesting, I went with direct imports. That also solves these issues.

I think the direct import is safe, and it makes the implementation much more easier 😃

@TorstenDittmann
Copy link
Owner

Good news 🎉. I think I've solved the issue with partials. The demo website works great now.

I noticed the changes in the PR #146 , using the extended type Schema & {...} to support alias import (Not sure how to call it in Typescript though 🤔). While it's interesting, I went with direct imports. That also solves these issues.

I think the direct import is safe, and it makes the implementation much more easier 😃

direct imports also fix the issue with barrel files (https://vitejs.dev/guide/performance#avoid-barrel-files). I'll try and see if I can review it tomorrow :-)

fix: check partial nodes by name's annotations
disable caching in the transformer for now, need to implement invalidate
cache mechanism.
@tcmhoang
Copy link
Contributor Author

tcmhoang commented May 1, 2024

Hey @TorstenDittmann , I've updated the test suite. Everything should be passing now, but there are a couple of matrix environments where it's still failing. Would you mind taking a peek at those? . NVM, I've fixed it. It looks like a champ. Do you mind giving it a quick once over?

@tcmhoang tcmhoang force-pushed the fix/tree-shaking branch 5 times, most recently from c03ed3a to 1ed9438 Compare May 1, 2024 11:16
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

2 participants