-
Notifications
You must be signed in to change notification settings - Fork 991
Clean up .js files in various TypeScript-first packages #2992
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
| import { getCoords, collectionOf } from "@turf/invariant"; | ||
| import { featureEach } from "@turf/meta"; | ||
| import { isObject } from "@turf/helpers"; | ||
| import { Feature, FeatureCollection, Point } from "geojson"; |
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.
isobands and isolines share these two matrix transform files, they're identical copies.
| var first = rhumbDestination(origin, cellSize * r, 0, { units: units }); | ||
| if (first.properties == null) { | ||
| first.properties = {}; | ||
| } |
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.
To satisfy the type strictness.
| @@ -1,56 +1,68 @@ | |||
| // Find self-intersections in geojson polygon (possibly with interior rings) | |||
| import { Feature, Polygon, Position } from "geojson"; | |||
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 file and simplepolygon felt really rough to migrate 😩
| output = { | ||
| type: "Feature", | ||
| geometry: { type: "MultiPoint", coordinates: output }, | ||
| }; |
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.
filterFn is always defined in our usage, so I removed this.
| } | ||
| >; | ||
| determineParents(output); | ||
| setNetWinding(output); |
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.
var output was declared twice and the types disagreed, and then the determineParents and setNetWinding calls were referencing it quasi-implicitly because of the closure. Gnarly stuff.
| for (var j = 0; j < pseudoVtxListByRingAndEdge[i].length; j++) { | ||
| for (var k = 0; k < pseudoVtxListByRingAndEdge[i][j].length; k++) { | ||
| var coordToFind = pseudoVtxListByRingAndEdge[i][j][k].coord; | ||
| let coordToFind = pseudoVtxListByRingAndEdge[i][j][k].coord; |
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.
^ multiple var defs bypassed by turning them into lets
| var pushing = { isect: nxtIsect }; | ||
| var pushing: { isect: number; parent: number; winding: number } = { | ||
| isect: nxtIsect, | ||
| } as any; // as any because parent and winding are filled in below |
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.
Leaving the code as-is, but I'd write this differently if we were TypeScript first. The isect/parent/winding tuple type is also defined in several places instead of giving it a name.
| break; | ||
| } | ||
| u[array[i]] = 1; | ||
| u[array[i].toString()] = 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 probably horrible for perf [number, number].toString() gets used as the key into u
|
|
||
| var key = isect; | ||
| var unique = !seen[key]; | ||
| var unique = !seen[key.toString()]; |
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 [number, number].toString() being used as a key. This has to be bad for perf.
Turns out the migration steps to TypeScript in certain packages just added typings to the .js files and moved on. This converts the rest of the supporting .js files, so we don't have to rely on the
tsupbuild behavior of copying these into the dist directory for us.I'll add a few inline notes of things I noticed along the way.