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

v7 alpha release #1499

Closed
rowanwins opened this issue Oct 2, 2018 · 15 comments
Closed

v7 alpha release #1499

rowanwins opened this issue Oct 2, 2018 · 15 comments
Milestone

Comments

@rowanwins
Copy link
Member

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

API Changes

  • There haven't been any changes to module API's

Higher level changes

  • Modules are now ES6 modules, written in plain ol' js, and built into a single package using rollup
  • Lerna is now no longer used for publishing as only a single package is published

Module enhancements

  • JSTS has been replaced in union, difference, intersect by polygon-clipping
    -dissolve has been overhauled
  • a number of other minor enhancements and fixes to modules...

Testing

  • My initial testing of the alpha bundle has been going ok
  • @Shongololo has raised an issue around ReferenceError: m is not defined when buffering linestrings.
    • need to do a bit of digging here, may be a minification issue or something else... although see note below, ideally I'm hoping to do away with the jsts dependency in the buffer module

Outstanding issues

  • The only module where jsts remains in use is buffer. I'm currently working on a replacement here
  • In the buffer replacement above I'm trying to build my library pulling in a couple of turf modules (eg coordEach). For some reason the build is pulling in all of turf rather than just the couple of modules I want (aka tree-shaking doesn't appear to be working...)
    • In my case I'm again using rollup as my build tool. I could perhaps investigate using webpack although it would be nice to have turf 'just working' irrespective of build tool choice.

Where to find it

What next

  • Try to resolve tree-shaking issue noted above
  • Publicise changes more widely

cc @stebogit @tmcw

@songololo
Copy link

songololo commented Oct 2, 2018

@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
Copy link
Member Author

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

@rowanwins
Copy link
Member Author

rowanwins commented Oct 9, 2018

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

  • it looks like when there are multiple exports from a single file (eg helpers or meta) it struggles to shake things properly
    • Problematic because pretty much every turf modules calls one or multiple of them
  • 3rd party module dependencies which get used in turf modules (eg concaveman) aren't being shaken

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
Copy link

songololo commented Oct 19, 2018

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
Copy link
Contributor

  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 #1428 (comment) 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
Copy link

Morgul commented Dec 6, 2018

@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
Copy link
Member Author

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
Copy link

sheerun commented Apr 17, 2019

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

@tmcw
Copy link
Collaborator

tmcw commented Apr 17, 2019

@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
Copy link
Collaborator

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

@zakjan
Copy link

zakjan commented May 28, 2019

I tried all ways of importing from turf ([email protected], @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
Copy link
Collaborator

tmcw commented May 28, 2019

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
Copy link
Member Author

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
Copy link

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
Copy link
Collaborator

twelch commented Jul 28, 2021

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.

@twelch twelch closed this as completed Jul 28, 2021
@mfedderly mfedderly added this to the 7.0.0 milestone Jul 28, 2021
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

No branches or pull requests