Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR converts the entire codebase from JavaScript to TypeScript, adding type safety throughout the library. The conversion was primarily done using GitHub Copilot with manual review and adjustments.
Key changes:
- Added comprehensive TypeScript type definitions and interfaces across all modules
- Configured TypeScript compilation with tsconfig.json targeting ESNext
- Updated build process to use TypeScript compiler instead of Rollup
- Removed old bundled distribution files in favor of TypeScript compilation output
Reviewed changes
Copilot reviewed 15 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | New TypeScript configuration with strict mode and ESNext targeting |
| package.json | Updated build scripts to use tsc, added TypeScript dependencies, changed main entry to compiled dist output |
| src/index.ts | Converted main entry with typed interfaces for TidePrediction API; contains API breaking change in getExtremesPrediction signature |
| src/node-corrections/index.ts | Added type definitions for node correction functions with AstroData parameter types |
| src/harmonics/prediction.ts | Added comprehensive interfaces for Timeline, Extreme, TimelinePoint and prediction options |
| src/harmonics/index.ts | Added typed interfaces for Harmonics factory and options |
| src/constituents/index.ts | Added Constituents interface defining all constituent types; uses Partial with non-null assertions |
| src/constituents/constituent.ts | Added Constituent interface with typed astronomy functions |
| src/constituents/compound-constituent.ts | Added CompoundConstituent and ConstituentMember interfaces |
| src/astronomy/index.ts | Added AstroData and AstroValue interfaces with typed astronomy calculations |
| src/astronomy/constants.ts | Converted to TypeScript exports of mathematical constants |
| src/astronomy/coefficients.ts | Added Coefficients interface and default parameter syntax |
| rollup.config.js | Removed - no longer needed with TypeScript compilation |
| dist/* | Removed old bundled distribution files |
| .gitignore | Added dist, TypeScript build artifacts to ignore list |
Comments suppressed due to low confidence (7)
src/harmonics/prediction.ts:143
- The type assertion to empty object (as Prediction) bypasses TypeScript's type checking. This object is later populated with methods, but initializing it this way could lead to runtime errors if the object is accessed before all methods are assigned. Consider defining all methods in the object literal initialization instead.
src/harmonics/index.ts:63 - Using any[] type for constituents array loses type safety. Since constituents have a known structure with _model and other properties (as defined in HarmonicConstituent interface), this should be typed as HarmonicConstituent[] or a more specific type that includes the _model and _phase properties.
src/node-corrections/index.ts:4 - The NodeCorrectionFunction type uses any[] for variadic arguments, which bypasses type checking. Consider using a more specific type or a union of allowed parameter types to maintain type safety, or use overloads for functions with different signatures like fModd and uModd that take a number parameter.
src/harmonics/index.ts:87 - The type assertion to empty object (as Harmonics) bypasses type checking. Consider initializing the object with all required methods in a single object literal to ensure type safety and avoid potential runtime errors if methods are called before assignment.
src/astronomy/index.ts:216 - Multiple type assertions using (as any) weaken type safety. These assertions are used when dynamically building the result object. Consider using a more type-safe approach, such as Record<string, AstroValue> for intermediate results and then casting to AstroData at the end after all required properties are populated.
src/constituents/index.ts:164 - Using Partial<Constituents> and then non-null assertions (!) throughout the code indicates a type safety issue. Consider initializing all basic constituents first before creating compound constituents, then export the complete object as Constituents without needing Partial or non-null assertions.
src/harmonics/prediction.ts:16 - The interface includes a catch-all index signature [key: string]: any that allows any property to be added to HarmonicConstituent. This weakens type safety and could allow typos or incorrect property names to go undetected. Consider removing this or being more specific about what additional properties are allowed.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #159 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 10 10
Lines 1038 1324 +286
Branches 0 94 +94
==========================================
+ Hits 1038 1324 +286 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Full disclosure: this was mostly done by Co-pilot in VSCode, with some guidance and changes from me. I will verify test counts and coverage before/after, and go through a careful review.