Typescript-ifying turf-great-circle

[thomas-hervey]

This change is intended to typescript-ify the turf-great-circles module following the typescript-ifying discussion https://github.com/Turfjs/turf/discussions/2538.

Summary of changes:

• Converted /index.js into /index.ts
• Added additional tests to /test.ts to cover different start and end input types
• Added /lib/arc.d.ts including class GreatCircle

Template requirements:

• [x] Is this a bug fix, new functionality, or a breaking change?
  • This is an update of the turf-great-circles module to use typescript.
• [x] Have read and followed the steps for preparing a pull request.

[thomas-hervey]

This is my first (draft) PR to TurfJS in preparation for other enhancement requests to turf-great-circle. @smallsaucepan given that you've begun typescript-ifying other modules, this PR should help contribute to this discussion https://github.com/Turfjs/turf/discussions/2538.

[smallsaucepan]

Fantastic. Thanks @thomas-hervey. Will take a look at this shortly.

[smallsaucepan]

@thomas-hervey it would be great if we could take the opportunity to do away with the whole lib/arc.* section and import as a third party module. There is https://www.npmjs.com/package/arc (which it looks like we've copy pasted into Turf at some point) but that doesn't have types.

Are you possibly in a position to submit types to DefinitelyTyped for the arc JS lib, so we can import it nicely into Turf as part of your PR? If not - and don't feel obliged - I'll give it a whirl and let you know when it's ready to integrate.

[thomas-hervey]

@smallsaucepan apologies for my delayed response. Yes I'll happily try to submit types to DefinitelyTyped for the arc JS lib and let you know if I run into any issues.

[smallsaucepan]

Hi @thomas-hervey. How did you go getting arc.js types added to DT?

[thomas-hervey]

Hi @smallsaucepan, I apologize again for the delay. I ran into some issues with DT and got sidetracked by my dissertation work. I should have reached out to you. I'd still like to submit a PR and should be ble to once I finish school in mid-June. However, feel free to give it a go first if you'd like.

[smallsaucepan]

No stress at all @thomas-hervey . Let us know when you're under way 👍

[thomas-hervey]

@smallsaucepan fortunately, I completed my dissertation work, and I can soon refocus my attention on this. I'll keep you posted.

[thomas-hervey]

@smallsaucepan, apologies for the delay. I'm happy to say that I've returned my attention here, and fortunately, the changes were not a heavy a lift.

Instead of submitting to DefinitivelyTyped, I opted to convert arc.js to TypeScript (see this PR awaiting review). However, I tested the integration locally and all tests and builds were passing.

The latest commits to this PR updated the branch with master and preemptively replaced the reference of lib/arc.js to a 0.2.0 arc package. I am now awaiting review of that PR, along with a version bump, before we can test this PR again.

Thanks!

[thomas-hervey]

@smallsaucepan following up on my previous comment, this PR is now ready for re-review. @springmeyer has released arc v0.2.0, which this PR is set up to use. Please take your time to review my changes.

Automated testing: All current /packages turf-great-circles tests passed. At the root all tests passed except for two suites, which appear to be failing on master:

2/230 targets failed, including the following:
      - @turf/standard-deviational-ellipse:test
      - @turf/directional-mean:test

Manual testing: I created a simple application with Leaflet where I visually inspected the results of several examples including: SF --> NY, short distances, an antimeridian crossing, a polar route, an equatorial route, start & end same location. For each of those examples I modified the start and end location as well as the parameters (ex. number of points). I tested this simple application locally using my Turfjs PR.

Thanks!

[thomas-hervey]

@smallsaucepan pinging you in case you missed a notification about my previous comment. This PR is ready for your re-review, but no time pressure on my end. Thanks for all of your feedback thus far!

[mfedderly]

I noticed that adding the dependency on the upstream arc package did slow down the benchmark a little bit. I'm happy to merge this for now, and if we do wind up with someone hitting perf issues we can re-vendor the code and grab it from here.

Thanks for working with the upstream to sort this out!

[smallsaucepan]

As above ☝️ Thanks for your thorough work on this @thomas-hervey

[thomas-hervey]

Thanks folks! Everything should work well, but let me know if you end up encountering any errors and I'll be sure to return and clean it up.

[springmeyer]

I noticed that adding the dependency on the upstream arc package did slow down the benchmark a little bit.

@mfedderly thanks for calling that out. The Arc.js package (both the version originally vendored and the latest) has not been carefully performance optimized. But that would be a completely reasonable thing to do, and I'm open to reviewing PRs along those lines. I imagine there can be some major efficiency improvements made if the API were adjusted and making it work better with Turf would be great.

[mfedderly]

has not been carefully performance optimized

I don't actually know the provenance of what happened to the Turf vendored version of the code and where it diverged from upstream, but the only obvious thing that's different is that the upstream is rounding results in GreatCircle.prototype.Arc. If someone does take an issue with the (overall pretty small) performance regression, I'll actually profile things and send a PR your way. I'm hoping it isn't necessary though. The tests seemed like they were doing ~120k ops/s on a Macbook Pro M4 which is hopefully fast enough in any case.