search

maplibre / maplibre-gl-js

MapLibre GL JS - Interactive vector tile maps in the browser
https://maplibre.org/maplibre-gl-js/docs/
Other
6.61k stars 712 forks source link

Tree shaking #977

Open birkskyum opened 2 years ago

birkskyum commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 days ago

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

birkskyum commented 2 days ago

@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 commented 1 day ago

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 commented 1 day ago

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 commented 1 day ago

@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 commented 1 day ago

@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).