Reduce unused code in UMD bundle
zakjan opened this issue · 14 comments
Current UMD bundle size is 60K (24K minified). Chrome DevTools Coverage tab reports 42.6% unused code, so in theory it could be lowered down up to 26K (10K minified) with the same dependencies.
I couldn't make Rollup tree-shaking working any better yet.
fyi: i took a stab awhile back at rewriting Terraformer and its two most popular plugins as ES modules without the primitives.
https://terraformer-js.github.io/terraformer/module-@terraformer_spatial.html
if you're interested in playing around with it, i'd be happy to help iron out the kinks and release @terraformer/spatial@2.0.0 if we can use it to help you import just intersects() and contains() to cut down your bundle size.
Wow, sounds great, where I can access the source?
Found it https://github.com/terraformer-js/terraformer/tree/master/packages/spatial ok I'll try to use it locally from a downloaded source and let you know how it works. Supporting plain GeoJSON is great!
One typo, swap the order of arguments in the second within call at https://github.com/terraformer-js/terraformer/blob/master/packages/spatial/index.js#L992
Tree shaking seems to work, UMD bundle size got down to 35K (14K minified), 3.6% unused code from Coverage tab, perfect.
Minor, not blockers:
intersects(polygon, point) returns true if a point is inside a polygon, but throws "Type Polygon to Point intersection is not supported by intersects" if it is not inside. It is correct false returned from within in this case. This issue exists also in the original Terraformer, I ignored it and forced to use contains with points (https://github.com/zakjan/leaflet-lasso/blob/master/src/calc.ts#L47-L49). What specific combination of types is actually not supported?
createCircle still returns incorrect radius (Esri/terraformer#321) and it is missing export. But I can stay with the other dependency.
TS would be nice.
down to 35K (14K minified), 3.6% unused code
very cool!
i haven't touched that project in ages, but hearing that it cut your bundle almost in half is just the encouragement i needed to get things cleaned up and actually put it on npm. I'd imagine it won't take me more than a week or two.
What specific combination of types is actually not supported?
i'm not sure if that's documented anywhere definitively or not. i'll look into it.
thanks for testing it and writing up a list of everything you saw.
i've released the packages. enjoy!
https://www.npmjs.com/package/@terraformer/spatial
https://jsbin.com/hepizok/edit?html
🎉 Can TS typings be reused from the previous library?
absolutely.
this time around i think the best option would be to have whoever is interested in maintaining/contributing publish them under @types. that works right?
Yes it works, but it's meant for legacy packages, which existed before TS and/or a maintainer is not interested in including typings in the original repo. Generally it's preferred to have them in the original repo, to prevent a split between typings and implementation. Up to you.
i've tried it both ways and tbh, the questions and requests for support with TS specific issues can be overwhelming for someone like me that doesn't write it often.
As far as Terraformer goes, we're lucky that the existing typings are quite good and the scope of the packages is small and the codebase is stable, meaning that it will remain small.
obviously there will be a little bit of work involved to repurpose the typings that already exist, but afterwards i'd expect very little effort to be required to keep them up to date.
if you're interested in taking ownership of them and providing support for TS issues directly in the repo for the next year or two, i'm happy to give you push access and place them side by side. if not, lets stick them in @types.
All right, I can own the typings in Terraformer. It's going to be simpler than the original typings, since all the removed classes.
sounds good. go ahead and fork the repo for now and open up a PR whenever you can spare the time.
Released in 2.1.0