-
Notifications
You must be signed in to change notification settings - Fork 992
Improve type inference in @turf/helpers' geometry and geometryCollection #2971
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
Conversation
| return multiLineString(coordinates).geometry as any; | ||
| case "MultiPolygon": | ||
| return multiPolygon(coordinates).geometry; | ||
| return multiPolygon(coordinates).geometry 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.
This as any casts are a little unfortunate. The case statements don't actually narrow our understanding of T, and so we need to reaffirm that we know what we're doing here.
| MultiPoint: MultiPoint; | ||
| MultiLineString: MultiLineString; | ||
| MultiPolygon: MultiPolygon; | ||
| }[T] { |
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 had considered making this a named interface and mapping against that instead (GeometryTypeNameToType), and referencing that instead of as any below, but I opted to not increase the public API for a mapping we only used in a single place.
| | "MultiPolygon", | ||
| >( | ||
| type: T, | ||
| coordinates: 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.
In a simliar fashion, we could narrow this any[] based on T, but that would be a breaking change, we can file it against the 8.0 milestone. I suppose you could also just make the case that we should do this in 7.x, because it is explicitly a bug in the caller if they aren't passing in the correctly shaped coordinates. My only concern with that is some amount of weirdness with number[] vs [number, number] issues, which might crop up depending on how consumers wrote their own types.
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 [number, number] thing is going to haunt us forever 😅
packages/turf-helpers/index.ts
Outdated
| return point(coordinates).geometry as any; | ||
| case "LineString": | ||
| return lineString(coordinates).geometry; | ||
| return lineString(coordinates).geometry satisfies LineString 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.
Does this need the satisfies as well? I wasn't sure if LineString is different from the others.
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 was an intermediate state but I decided that the satisfies step didn't really add anything and I removed it (or at least I thought I removed it).
|
Looks good @mfedderly. While trying out a couple of things, I found the below a shade more readable. Produces the same result though, so only mentioning it in case you agree on the look of it. The helper doesn't need to be exported so shouldn't affect the public interface. type GeometryByType<T extends Geometry["type"]> = Extract<
Geometry,
{ type: T }
>;
export function geometry<
T extends Exclude<Geometry, GeometryCollection>["type"],
>(
type: T,
coordinates: any[],
_options: Record<string, never> = {}
): GeometryByType<T> {
switch (type) {
case "Point":
return point(coordinates).geometry as GeometryByType<T>;
case "LineString":
return lineString(coordinates).geometry as GeometryByType<T>;
case "Polygon":
return polygon(coordinates).geometry as GeometryByType<T>;
case "MultiPoint":
return multiPoint(coordinates).geometry as GeometryByType<T>;
case "MultiLineString":
return multiLineString(coordinates).geometry as GeometryByType<T>;
case "MultiPolygon":
return multiPolygon(coordinates).geometry as GeometryByType<T>;
default:
throw new Error(type + " is invalid");
}
} |
I think this is actually untrue. Microsoft built a tool called api-extractor with a rule for exactly this circumstance. The docs page explains some of the pitfalls of not exporting referenced types. https://api-extractor.com/pages/messages/ae-forgotten-export/ |
Which largely wouldn't apply in our case i.e. an internal type inference helper. Even taking API extractor warnings as gospel, it doesn't have to be defined as a type - inlining |
Cherry-picking some of the work from #2970 so that we can merge it today instead of waiting for a major release.
Specifically we can further narrow the return types of
geometryandgeometryCollectionbased on what the user sends in the arguments.