-
Notifications
You must be signed in to change notification settings - Fork 66
Typescript conversion #55
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
Typescript conversion #55
Conversation
|
@springmeyer - This PR adds TypeScript support while maintaining full backward compatibility. This should be useful in any downstream Typescript projects. This work was motivated during the process of type-ifying the TurfJS great-circle package (see this related PR). |
|
Thank you. I will review soon. |
- Fix support for browser bundle (arc.js) with esbuild for UMD support - Rename source files to *-class.ts to avoid naming conflicts with browser bundle - Add backward compatibility tests (24 tests) - Reorganize test suite - Update README with extended usage examples - Remove obsolete compiled test directory - Maintain API compatibility
|
Heads up @springmeyer, per my last commit message, I made a few additions to ensure backwards compatibility for browser usage of a root |
springmeyer
left a comment
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.
@thomas-hervey this looks great, I'm sure this can merge soon. Thank you for the heavy lifting here. This had been on my list to do (typescript and better module support) but I've been focused on other things recently.
A few questions & suggestions:
- Can you add a DEVELOPING.md that sketches out how to work with the TS code (test locally, add new types, run the build, and particularly anything I need to do to publish and have this work correctly)
- Does the current
.npmignorelook good and therefore ./dist will get published now?
- Does the current
- Why all the renames of the source files from
name.jstoname-class.js? Is that absolutely needed? If not, it creates diff noise that I'd like to avoid. If it is needed, cool. - is it a best practice to have the tests depend on the
./dist? In my experience I've normally had the tests be re-written to ts and then just use a ts-saavy testing toolkit like jest. I'm 100% fine with your solution here as is, just wanted to check / encourage you to port to TS if that would make things easier.
• Remove -class suffix from source files (arc-class.ts → arc.ts, etc.) • Convert all tests to TypeScript with Jest + expect-type for type safety • Simplify test scripts and remove tape dependency for cleaner setup
|
@springmeyer thank you for your prompt feedback. I'll keep my response short.
Take a look and let me know if you spotted any mistakes or other bad patterns. Overall, I think we should be good to publish this soon. |
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.
Pull Request Overview
This PR converts arc.js from JavaScript to TypeScript while maintaining 100% backward compatibility. The conversion includes full type definitions with GeoJSON standard compliance, a dual build system producing both CommonJS and ESM outputs, and significantly expanded test coverage. All existing JavaScript code continues to work unchanged while enabling new TypeScript usage patterns.
- TypeScript conversion with strict compiler settings and modern module formats
- Enhanced test suite reorganized from 25 to 102 tests with integration and type validation
- Dual output system supporting CommonJS, ESM, and browser bundle formats
Reviewed Changes
Copilot reviewed 19 out of 64 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.esm.json | ESM-specific TypeScript configuration |
| test/typescript.test.ts | TypeScript-specific tests for type safety and inference |
| test/integration.test.ts | Complex real-world route integration tests |
| test/great-circle.test.ts | GreatCircle class functionality tests |
| test/coord.test.ts | Coordinate class unit tests |
| test/build-output.test.js | Build output validation tests for CommonJS and browser bundle |
| test/arc.test.ts | Arc class functionality tests |
| test/arc.test.js | Original JavaScript test file removal |
| src/utils.ts | Utility functions including coordinate rounding and constants |
| src/types.ts | TypeScript type definitions with GeoJSON compliance |
| src/line-string.ts | Internal LineString class for geometry building |
| src/index.ts | Main module exports for dual CommonJS/ESM support |
| src/great-circle.ts | Great circle calculation class with TypeScript types |
| src/coord.ts | Coordinate class with longitude/latitude handling |
| src/arc.ts | Arc class for great circle calculation results |
| package.json | Updated build system with TypeScript tooling and dual outputs |
| arc.js | Generated browser bundle from TypeScript compilation |
| README.md | Enhanced documentation with TypeScript usage examples |
| DEVELOPING.md | Development guide for TypeScript workflow |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
springmeyer
left a comment
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.
Great followup changes, this looks good. I will merge and publish shortly.
|
@thomas-hervey after merging I noticed a problem: The example index.html references I can fix that by doing: diff --git a/package.json b/package.json
index 8c174e8..9470ab4 100644
--- a/package.json
+++ b/package.json
@@ -39,7 +39,7 @@
},
"scripts": {
"build": "npm run clean && tsc && tsc -p tsconfig.esm.json && npm run build:browser",
- "build:browser": "esbuild src/index.ts --bundle --format=cjs --outfile=arc.js --platform=node",
+ "build:browser": "esbuild src/index.ts --bundle --format=iife --global-name=arc --outfile=arc.js --platform=browser",
"clean": "rm -rf dist/ && rm -f arc.js",
"test": "jest",
"test:all": "npm run test && npm run test:build",
But I gather that is not what you had in mind for the |
|
@springmeyer great catch and apologies for missing the Let me know if you have any questions. |
This PR converts arc.js from JavaScript to TypeScript while maintaining 100% backward compatibility.
Changes
JavaScript Usage
TypeScript Usage
Testing