-
Notifications
You must be signed in to change notification settings - Fork 992
Migrate @turf/meta to TypeScript #2981
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
base: master
Are you sure you want to change the base?
Conversation
| geometryIndex: number | ||
| ) => void | false, | ||
| excludeWrapCoord?: boolean | ||
| ): void | false { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Public API change!
The index.d.ts version of this had just void for coordEach and the callback return value.
I had to add | false to every *Each method's callback, and also to some of the *Each functions themselves.
This is the first instance of the change, but it happens throughout the file on most *Each functions.
I think this is ok to do without a major version because returning void is still allowed.
| stop = isFeatureCollection ? geojson.features.length : 1; | ||
| stop = isFeatureCollection | ||
| ? (geojson as FeatureCollection).features.length | ||
| : 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also an example of another really common change in this PR. TypeScript was not narrowing the types enough and so we require some manual annotation of types to make everything work out. I don't think I'm doing anything in here that isn't being checked at runtime.
| featureIndex: number, | ||
| featureProperties: P, | ||
| featureBBox: BBox | undefined, | ||
| featureId: Id | undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Public api change!
These are just being pulled from the underlying features, which may or may not have a bbox or id. This change can probably cause new TypeScript errors, but these things are potentially already runtime errors so we can probably push it without a major version? Worst case people can revert to the existing behavior by using ! and move along with the upgrade.
b08b0be to
4a38f34
Compare
| featureIndex: number, | ||
| featureProperties: P, | ||
| featureBBox: BBox | undefined, | ||
| featureId: Id | undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another instance of the bbox/id getting new undefined options as above
| excludeWrapCoord | ||
| ); | ||
| return previousValue; | ||
| return previousValue as Reducer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block of three as Reducer casts are a bit of a compromise to avoid making breaking changes right now.
In the first cast, currentCoord is almost certainly not a Reducer (its the first coord, kind of in the way Array.prototype.reducer will fill in accumulator with arr[0] if an initial value is unspecified).
The second cast in theory could be a ! because of previousValue being set to currentCoord, except that's slightly wrong because Reducer could extend undefined, so I think as Reducer is in the right direction?
The last cast in the return is also wrong because previousValue can remain unset if initialValue is unspecified and we don't actually have any coords to reduce in geojson.
So all of these casts are slightly wrong, but I don't actually think we can change it without changing the runtime behavior to make this simpler to type. I think my preference would be to make initialValue mandatory, and remove the code that sets previousValue if it was not already set. That would make the types cleaner and users could bring the behavior back by checking the *Index value in the callback.
This is just the first instance of this change, but the logic basically holds across all of the rest of reducers below as well.
4a38f34 to
c4709a8
Compare
…ex.d.ts into index.ts
Public API change: callbacks returning void | false Public API change: geomEach callback bbox and id args can be undefined Hand-annotate types inside branching that isn't being inferred by TypeScript
c4709a8 to
4bfde1b
Compare
| return null; | ||
| case "Point" as any: | ||
| case "MultiPoint" as any: | ||
| return null as any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Point and MultiPoint cases here aren't technically reachable, but I'm afraid to change runtime behavior.
|
|
||
| // Find SegmentIndex | ||
| if (geometry === null) return null; | ||
| if (geometry === null) return null as any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
findPoint and findSegment both have null return paths that aren't documented in the type signature
|
While working on this, I found a bunch of stuff that probably needs to get sorted as part of a major version. I'm considering the wisdom of actually just deprecating several of these methods instead of trying to fix them. |
| coords.push(currentCoords); | ||
| return currentCoords; | ||
| }, line[0]); | ||
| }, undefined as any); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was a legitimate bug in the tests
| test("unknown", (t) => { | ||
| t.throws(function () { | ||
| meta.coordEach({}); | ||
| meta.coordEach({} as any, () => undefined); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It throws an error on the invalid {} arg, but TypeScript requires the 2nd arg to be passed anyhow now
| test("flattenEach -- MultiPoint", (t) => { | ||
| featureAndCollection(multiPt.geometry).forEach((input) => { | ||
| const output = []; | ||
| const output: MultiPoint[] = []; // this is actually Point[] but flattenEach's types are bad |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flattenEach and flattenReduce both have pretty gnarly typing bugs that need to be fixed. MultiPoint/LineString/Polygon should all get transformed to non-multi Point/LineString/Polygon.
| return current; | ||
| }, | ||
| null | ||
| null as any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The generic is inferring exactly null for the type, which would make the return current invalid without this any cast
| fc, | ||
| (prev, feature, featureIndex) => prev.concat(featureIndex), | ||
| [] | ||
| [] as number[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with these, but its a never[] instead
| const featureIndexes: (number | undefined)[] = []; | ||
| const multiFeatureIndexes: (number | undefined)[] = []; | ||
| const geometryIndexes: (number | undefined)[] = []; | ||
| const segmentIndexes: (number | undefined)[] = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a bunch of these functions in meta that pass optional *Index values to the callback, when I think they're always specified and not optional. We need to do a pass and clean that up so this sort of thing can be number[] as much as possible.
| meta.lineEach(pt as any, noop); // Point geometry is supported | ||
| meta.lineEach(multiPt as any, noop); // MultiPoint geometry is supported | ||
| meta.lineReduce(pt as any, noop); // Point geometry is supported | ||
| meta.lineReduce(multiPt as any, noop); // MultiPoint geometry is supported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More instances of TypeScript being annoying when we try to test runtime behavior that TypeScript would have blocked.
| */ | ||
| const coordEachValue: void = meta.coordEach(pt, (coords) => coords); | ||
| coordEach(pt, (coords, index) => coords); | ||
| meta.coordEach(pt, (coords, index) => coords); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this was only kinda-sorta a working syntax before, but once we change the package to TypeScript we start getting errors about it. coordEach's callback should return void | false, but specifying coords like this makes is a return value. I just add {}s around each of these to just make it a no-op reference inside the function instead of a return value. This gets repeated all the way down the file.
I think that this would not have been allowed if you were already running TypeScript against this file before, and this isn't a real break. It was pretty annoying to have to eat this syntax change though, so if you think it would have been allowed before we should figure out how to not cause this.
Steps:
typevery wellbboxandidcallback args as potentiallyundefined, because they areReducegeneric to get us to the next major version where we can fix things