Skip to content

[editable-layers] Update to turf 7 and use geojson types#447

Merged
charlieforward9 merged 16 commits intovisgl:masterfrom
timnyborg:turf-7-and-geojson
Nov 24, 2025
Merged

[editable-layers] Update to turf 7 and use geojson types#447
charlieforward9 merged 16 commits intovisgl:masterfrom
timnyborg:turf-7-and-geojson

Conversation

@timnyborg
Copy link
Contributor

Rebased and updated #221

@timnyborg timnyborg marked this pull request as ready for review November 22, 2025 19:12
Copy link
Collaborator

@charlieforward9 charlieforward9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A hefty lift! Thank you for this. If we can resolve these points below, I think we can have this merged in by today.

Great to see all the @ts-expect-errors go away.

Im seeing a TON of FeatureCollection<SingleGeometry> instances, this would be better as a shorthanded, single export util coming from the same file these types are defined. Cleans up dependent files with a single export rather than two. Open to just about any name that is a shorthand of this.

Can you explain the removal of all the files in modules/editable-layers/src/lib/ and the curve-utils? Are they not used anywhere else across the repo or exported? (cc. @ibgreen, @georgioskarnas)

Also seeing a test fail from the geometry refactor.

@timnyborg
Copy link
Contributor Author

Can you explain the removal of all the files in modules/editable-layers/src/lib/ and the curve-utils? Are they not used anywhere else across the repo or exported? (cc. @ibgreen, @georgioskarnas)

From the original PR (I should copy the description across ):

Drops code from long, long dead lib folder, and an unused curve-utils file, rather than trying to update turf usages there

A bunch of stuff from lib was exported (see the removals in index.ts), but none of it is relied on in the rest of the package, or referred to anywhere in the repo or the old nebula.gl documentation. As far as I can tell, it's all dead.

@timnyborg
Copy link
Contributor Author

Im seeing a TON of FeatureCollection<SingleGeometry> instances, this would be better as a shorthanded, single export util coming from the same file these types are defined. Cleans up dependent files with a single export rather than two. Open to just about any name that is a shorthand of this.

Swapped for GeometryFeatureCollection. There's some additional cleanup that could be done across the repo to point Feature & FeatureCollection imports directly to geojson, but I tried to keep this PR focused on the actual change as much as possible, so I'd suggest we just follow up.

@charlieforward9
Copy link
Collaborator

Excellent. Thank you for your time. Gonna pull a senior collaborator in here for a final check, and once they confirm, we'll see this finally land.

"@turf/transform-rotate": "7.2.0",
"@turf/transform-scale": "7.2.0",
"@turf/transform-translate": "7.2.0",
"@turf/union": "7.2.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just spotted these are exact versions. Will revert to ^

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks very good, thanks for putting this together.

Please consider the type renaming proposals and let me know.

// Feature types
export { Feature, FeatureCollection } from 'geojson';

export type BoundingBoxArray = [number, number, number, number];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great to keep the file in sections, so we don't mix Geometry defs, Feature defs, other type defs, so that a user can quickly scan all exports of a given category.

e.g.

// Geometry exports

// Feature exports

// Other type exports

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After doing some more types clean-up, I'm not sure there's much value in grouping in that fashion - we're left with very little: 4 Simple* types (best understood together, I'd argue), PolygonGeometry, and AnyGeoJson.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plus a bunch of geojson re-exports I want to remove in a follow-up

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I would like to see one line TSDoc comments for all the new types. These show up in vscode and makes it easier for users.

  • A note in what's new that we are now using turf 7 would be nice.
  • A doc page for geojson types is not P0 but wouldn't be unreasonable, mentioning that we are using @geojson/types with extensions.
  • Also may want to make sure we are using latest @types/geojson (perhaps that is already the case)
/** Simple geometries (excludes GeometryCollection) *?

@timnyborg
Copy link
Contributor Author

I think I've addressed all the comments above. There's a lot of noise in the commit history now, so I'd suggest we squash.

@charlieforward9
Copy link
Collaborator

Incredible work. Squash and merge is default. Thanks for getting to this so quickly.

@charlieforward9 charlieforward9 merged commit e7edaca into visgl:master Nov 24, 2025
1 check passed
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