Increase strictness of @turf/line-chunk input #2970
Conversation
The method only works correctly for LineString and MultiLineString shaped geometries (with any containing Features, Collections, etc. This makes the strictness more explicit in the docs, the TypeScript types, and runtime checking.
| ): Feature<GeometryCollection, P> { | ||
| const geom: GeometryCollection = { | ||
| ): Feature<GeometryCollection<T>, P> { | ||
| const geom: GeometryCollection<T> = { |
There was a problem hiding this comment.
This change has an interesting backstory.
packages/turf-line-chunk/types.ts has lineChunk(geomCollection, 2);, but geometryCollection was erasing the specific subtype (the default is just GeometryCollection<Geometry>) instead of inferring them forward into the return type. This changes geometryCollection to preserve any sort of information we did have for the things getting put into the collection.
I believe that this class of type strictness error may hit other consumers if they are creating geometry collections using other methods.
| throw new Error( | ||
| "Only LineString and MultiLineString geometry types are supported" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Open to handling points differently here, but this has the nice property of aligning the TypeScript strictness with the runtime behavior.
There was a problem hiding this comment.
In general, what do we want Turf's runtime behaviour to be when it's gets unexpected / inappropriate input? Halt on unexpected input? Or soldier on silently (or with a warning)?
There was a problem hiding this comment.
I think about this along a few axes.
The first is something like "what is this method supposed to accept?", in this case we have a method that only makes sense on line-shaped things, so points and polygons should be considered invalid and strongly rejected. But we should take a more moderate stance on the container shape. Send a LineString directly, or a FeatureCollection<Feature<GeometryCollection<MultiLineString>>> if you want, and we'll try to handle it.
The second is the type checking errors vs runtime errors. Ideally these would be consistent, but I think its safer to add strictness at type checking time (it won't surprise the consumer in production). We're flagging things that are likely already buggy at runtime for them anyhow. At one point we started removing the runtime checks because we added guards at the type level, but I'm not sure we'll ever be able to get away with that because the runtime guards provide way clearer feedback to the dev than some error deep within the implementation.
I'm not a big fan of console.warn or similar from within a library, large apps may have their own logging systems which that make this form of feedback pretty difficult to manage (do you shim out console before calling some arbitrary function just to grab the message and repeat it into your own logging?). It also seems less likely to get noticed and fixed than just being loud and tossing an Error.
Overall I think the current state of this PR reflects my idea of what 'ideal' looks like in this case, but I understand that we might be reluctant to actually merge this without a major release because of the risk of runtime breakage. I would certainly consider removing the runtime error for Point-likes, and changing that to a warn or keeping it silent for now, and coming back with yet another PR when we do a major break. You could even make the case that the potential TypeScript errors from making the GeometryCollections more strict would itself require a major bump, and therefore we can shelve this entire PR for the 8.x milestone and revisit it later.
We have a pretty limited velocity on this project, and blocking little improvements like these until the next major winds up delaying the improvements getting rolled out to users. I'm not sure what the right balance is between getting things out quickly vs disrupting the folks that are tracking our releases more closely.
There was a problem hiding this comment.
points and polygons should be considered invalid and strongly rejected. But we should take a more moderate stance on the container shape
This seems a good balance. Caller has to be more alert about what they pass in. So for a geometry collection containing 99 lines and 1 point, we would throw?
coming back with yet another PR when we do a major break
As is I think this would have to be a major release candidate:
- now throws where we previously silently ignored invalid geometries
- narrower input types could break builds
To keep moving forward between majors then, do we make this PR about the helpers.geometryCollection part? That doesn't narrow input types, and doesn't broaden the return type either. Runtime behaviour would stay the same.
There was a problem hiding this comment.
Sure. I'll send this to the v8 milestone and go look at everything in helpers tomorrow to see if there's anything else that can get a similar treatment.
There was a problem hiding this comment.
Got eager and broke that out here #2971
|
Note to future me: you need to regenerate the readme 😆 |
The method only works correctly for LineString and MultiLineString shaped geometries (with any containing Features, Collections, etc. This makes the strictness more explicit in the docs, the TypeScript types, and runtime checking.
Follow up to #2969