Migrate @turf/line-chunk to TypeScript #2969
Conversation
| results.push(segment); | ||
| }); | ||
| sliceLineSegments( | ||
| feature as Feature<LineString>, |
There was a problem hiding this comment.
I think this is a bit of a sketchy cast, but I can't change this without changing the input signature which also takes GeometryCollection and Feature<GeometryCollection>, which theoretically allow more geometry types. I think we just pass through non-line geometries though 🤷 . Kind of a weird API.
There was a problem hiding this comment.
Quickly checked and the code throws at runtime if passed a polygon, and fails silently if passed a point. Do we add a runtime check to avoid the throw for polys (fixing a bug), and print a warning for everything that won't be handled e.g."Passed a <geometry.type> to lineChunk, probably in your GeometryCollection, ignoring."?
Longer term I'm pretty sure we can narrow to GeometryCollection<T> and Feature<GeometryCollection<T>>
There was a problem hiding this comment.
Ah I tested Point and it didn't throw an error, so I was hesitant to change the runtime behavior by starting to throw one. Lets land the js -> ts migration and I'll come behind and make the GeometryCollection typings more strict and look at what to do for the unsupported geometry types in a separate PR (for easier revert, if need be 🤞).
There was a problem hiding this comment.
starting to throw one
Wasn't suggesting that. Leave point as is and guard against polygon throwing.
There was a problem hiding this comment.
Yeah lets continue discussing here: #2970 (comment) :)
Happy to do either way though.
| { units: units } | ||
| ); | ||
| callback(outline, i); | ||
| callback(outline); |
There was a problem hiding this comment.
I just standardized on having no index arg since it wasn't being used and the calls conflicted a bit.
smallsaucepan
left a comment
smallsaucepan
left a comment
There was a problem hiding this comment.
Looks great thanks @mfedderly. Suggested a runtime check / warning we could add if you think it'd be a good idea.
Another quick package migration to work towards unifying the build experience