Turfjs / turf

A modular geospatial engine written in JavaScript and TypeScript
https://turfjs.org/
MIT License
9.21k stars 934 forks source link

v7 alpha release #1499

Closed rowanwins closed 3 years ago

rowanwins commented 5 years ago

Inline with the suggestions contained within #1428 I've been working on getting a v7 release ready (see #1432)

API Changes

Higher level changes

Module enhancements

Testing

Outstanding issues

Where to find it

What next

cc @stebogit @tmcw

songololo commented 5 years ago

@rowanwins this is a huge step forward, thank you.

Regarding tree shaking, I'm going by memory, but I believe that it didn't work with webpack either. I'll update this with further comments if I figure anything out.

Update: Looked again and it seems webpack production mode is bundling what I assume to be the entire packaged turf.min.js file instead of extracting the relevant es modules from js. Might be a rollup configuration issue?

rowanwins commented 5 years ago

Yep confirmed webpack also isn't succesful in tree-shaking, need to do some more digging on the turf config end...

rowanwins commented 5 years ago

Hmm so I've done some more digging and it found a few things

So a few options I can think of

  1. Accept that it won't shake as well as I'd like and move on
  2. Split those files with multiple exports out into single files (eg could still have a helpers directory, but put the featurecollection module in one file, and the feature in another)
    • I suspect this would help with some of the shaking but wouldn't fix those 3rd party imports...

Have tried hunting around for similar libraries but the ones I can think of (namely simple-statistics and lodash) don't have any dependencies.

Any brilliant ideas @tmcw ?

PS This is where the lerna mono-repo stuff was helpful :)

songololo commented 5 years ago

In case it helps, browsing through rollup's docs they have a suggestion regarding false-positives - to import using import map from 'lodash-es/map style vs. import { map } from 'lodash-es')

You can often mitigate those false positives by importing submodules (e.g. import map from 'lodash-es/map' rather than import { map } from 'lodash-es').

PS - More listed under danger zone on fine-tuning tree-shaking using treeshake.propertyReadSideEffects property.

andrewharvey commented 5 years ago
  1. Split those files with multiple exports out into single files (eg could still have a helpers directory, but put the featurecollection module in one file, and the feature in another) I suspect this would help with some of the shaking but wouldn't fix those 3rd party imports...

As I mentioned at https://github.com/Turfjs/turf/issues/1428#issuecomment-401651415 anything to de-group helpers/meta etc. is :+1 in my opinion.

Looking forward to being able to just use:

import { feature, featureEach } from `@turf/turf`
Morgul commented 5 years ago

@rowanwins If you're still having problems, you might reach out to the OpenLayers devs, they made their v5 work correctly with treeshaking in webpack, rollup and parcel. (v4 had the same problems you seem to be running into).

I'll bet they'd be more than willing to share their experiences.

As a side note, right now Turf is a full 40% of my current webpack bundle for my application. (I found this issue looking for issues about tree shaking in turf.) I'm extremely excited about the v7 changes.

rowanwins commented 5 years ago

Hi @Morgul

Thanks for the suggestion. We have actually made a bit of progress on the treeshaking front thanks to some advice from @tmcw so I think we're back on track. Basically the issues primarily relate to some of our 3rd part dependencies so I've been doing some work on upstream libs and inlining others to make them more friendly for us.

So stay tuned :)

sheerun commented 5 years ago

Our application size is suffering from jsts. Maybe some help rewriting buffer? Could you point to something?

tmcw commented 5 years ago

@sheerun Take a look at #88, which is the master issue for removing JSTS. It's kind of like the 'sword in the stone' for this project.

dpmcmlxxvi commented 5 years ago

@rowanwins Just wondering, what is the status on v7?

zakjan commented 5 years ago

I tried all ways of importing from turf (turf@7.0.0-alpha.1, @turf/turf, @turf/boolean-contains), but tree-shaking doesn't work for any of them with Rollup, not even with Rollup dangerous switches. Visualized with rollup-plugin-visualizer, full @turf/meta and @turf/helpers are included, which are probably unnecessary.

I'm sorry, but this is a blocker for me for building a lightweight browser library https://github.com/zakjan/leaflet-lasso Until turf supports tree-shaking properly, for browser use I need to look for another library.

tmcw commented 5 years ago

Folks, if you can help out please do, but please don't threadsit on these issues. The status of v7 is readable in the original post of this issue, in the PR linked in the main thread, along with the commits therein. Per those issues, getting everything tree-shakeable is still something that's being worked on, so you shouldn't expect it to work yet: it won't.

@zakjan It looks like your library just needs a point in polygon method. You'll find many standalone modules for that by googling or searching npm: for example, point-in-polygon.

rowanwins commented 5 years ago

Hi @zakjan & @dpmcmlxxvi

There are a few more unresolved issues around the polygon-clipping library that are preventing it from shaking properly, for example I think this use of the singleton pattern, as well as these variables and functions. I have a version locally which shakes however it breaks a lot of tests which would need to be refactored accordingly.

With tree-shaking I think the meta and helper modules these will be relatively low priority as minified they come out very small.

I've also released point-in-polygon-hao recently which works with polygons containing holes out of the box as well as degenerate polygons (compared to point-in-polygon).

AzRieilStuff commented 3 years ago

I've tried 7.0.0-alpha.1 and there were a lot of glitches with buffer which I applied to countries geojson ( not depends on radius or any other params, its just break borders into large triangles over all map ). Revert to 6.3.0 solved problems

twelch commented 3 years ago

Closing, as this ambitious v7 rewrite effort was dropped, and it's PR #1432 has been closed.

The effort has been superseded by continued and smaller 6.x minor releases.