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

Tree shaking #977

Open
birkskyum opened this issue Feb 10, 2022 · 9 comments
Open

Tree shaking #977

birkskyum opened this issue Feb 10, 2022 · 9 comments
Labels
enhancement New feature or request PR is more than welcomed Extra attention is needed

Comments

@birkskyum
Copy link
Member

As I understand, tree shaking is not properly supported currently. I was thinking we could make this a tracking issue and get an overview for what needs to happen in order to get it working.

@javlund
Copy link

javlund commented Jun 22, 2022

Any news on this one? This lib is by far the biggest chunk in our setup, and according to Chrome coverage it seems that we're only using half of it. However, the number increased from 30% to 50% just by clicking around on the map etc., so I don't know if it's even viable to do this.

@HarelM
Copy link
Collaborator

HarelM commented Jun 22, 2022

Not really, the current situation, where the library is wrapped so that you don't even need to configure the worker threads is conflicting with tree shaking.
Having said that, there's probably a way to facilitate for this, but it requires someone who's an expert in this area which I don't think we have maintain this project.
If you would like to push this forward please do, I'd be happy to help as much as I can. :-)

@chanmathew
Copy link

hi @HarelM - just wondering if there has been any updates on this or if this is still currently blocked?

@birkskyum
Copy link
Member Author

birkskyum commented Nov 7, 2024

@chanmathew , I believe the latest update in this regards is that there not too long ago was a large refactor towards named imports, allowing unused pieces to be left out.

We'd be able to improve our support for tree shaking by distributing an ESM bundle, but that requires all the dependencies to be updated from CJS to ESM too. There is a blocker here, related to updating the dependencies, because this one has some issues, and resolution is on hold due to a parental leave:

Another piece of the puzzle is tooling. Our test harness uses Jest, which only has experimental ESM support. Last time I tried running our bundle tests with jest and ESM were there some issues. I'm hoping that the upcoming Jest 30 will improve on the situation, but should it persist when the rest of the system is in place, is there preemptively prepared a PR here than can readily swap Jest for the ESM-ready Vitest:

@HarelM
Copy link
Collaborator

HarelM commented Nov 7, 2024

The main problem from my point of view is how to share code between the workers and the main thread.
This is currently relaying on how this is build using commonJs and I don't know how to do it with ESM...
So, if the purpose of ESM is tree shaking, but you get the bundle size duplicated it totally beats the purpose...

@knd775
Copy link

knd775 commented Nov 7, 2024

So, if the purpose of ESM is tree shaking, but you get the bundle size duplicated it totally beats the purpose...

It increases the package size, but the actual js bundled in an application wouldn't increase with any half-decent bundler

@jasongitmail
Copy link

@birkskyum Vitest is amazing. It's much faster than Jest and is a good move on its own merit. I switched from Jest to Vitest years ago and never looked back, and it's only going to grow in usage with Evan You now creating VoidZero, which Vitest is part of.

@birkskyum
Copy link
Member Author

birkskyum commented Nov 7, 2024

@jasongitmail , if you find Vitest be much faster already now (before rolldown etc.), could you maybe help debug the config of the PR i linked? I'm asking because I haven't managed to figure out why our test-unit suite takes for Vitest 2 = 12s, and Jest (we compile with tsc, not swc) = 8s, across mac (locally) and linux/win (CI).

@jasongitmail
Copy link

@birkskyum I'll drop a comment on the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request PR is more than welcomed Extra attention is needed
Projects
None yet
Development

No branches or pull requests

6 participants