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

WIP: use rollup #256

Closed
wants to merge 8 commits into from
Closed

WIP: use rollup #256

wants to merge 8 commits into from

Conversation

themre
Copy link
Contributor

@themre themre commented Nov 24, 2017

I created a quick branch to use rollup. I added turf functions. it's still big because it's without uglify (have this problem Turfjs/turf#1086), but it's size went from 1.3 to 0.95MB. also CSS images I copied next to CSS and didn't bundled them into CSS, because I didn't have time for embeding them.

@themre themre mentioned this pull request Nov 24, 2017
@themre
Copy link
Contributor Author

themre commented Nov 26, 2017

well... @DenisCarriere did awesome job in converting turf modules to ES. I extracted some methods from jsts to separate utils file. final bundle is now almost 300kB smaller (final is 284 kB). webpack is not suitable for this, bundle is 634 kB big.

@DenisCarriere
Copy link
Contributor

👍 Sweet! Glad to hear it worked out, ideally we can keep making it smaller and smaller since there's a lot of dead code laying around a few dependencies, but it's a step forward!

@codeofsumit
Copy link
Contributor

codeofsumit commented Dec 1, 2017

@themre @DenisCarriere great to see progress in this but I still think these optimization should be upstream in the dependency tree. So either turf or jsts as I don't think leaflet.pm's build should optimize it's dependencies, but only it's own code.

@DenisCarriere is that something you will incorporate in the served turf bundle or would you suggest to put it into leaflet.pm (and essentially any other library that uses turf)?

@DenisCarriere
Copy link
Contributor

@codeofsumit All of these optimizations will be available in the next TurfJS release as CommonJS/ES Modules & UMD bundles, right now there's still a bit of tweaking on refactoring turf-jsts (for internal TurfJS purposes) later on these changes would one day land in jsts.

I wouldn't recommend adding your own jsts code in leaflet.pm since you won't see any optimized gains compared to importing ES modules from TurfJS, it will be exactly the same bundle size.

Ideally we would love to drop jsts entirely, however it's been proven VERY difficult to do so.

These are the modules which directly & indirectly depend on JSTS:

// JSTS Modules
export {default as difference} from '@turf/difference';
export {default as buffer} from '@turf/buffer';
export {default as union} from '@turf/union';
export {default as intersect} from '@turf/intersect';

// JSTS Sub-Models
export {default as dissolve} from '@turf/dissolve';
export {default as hexGrid} from '@turf/hex-grid';
export {default as mask} from '@turf/mask';
export {default as squareGrid} from '@turf/square-grid';
export {default as triangleGrid} from '@turf/triangle-grid';
export {default as interpolate} from '@turf/interpolate';

If you import any of these modules, you're bundle will increase by ~500KB.

Once TurfJS v5.1 is published, you can have a look at any more optimization for bundle sizes, but JSTS would be at the "smallest" size possible at the moment.

@codeofsumit
Copy link
Contributor

guys, thanks for your work on this but I will close this now as the size optimizations should happen upstream in the dependency tree and leaflet.pm will only optimize it's own code.

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.

3 participants