-
Notifications
You must be signed in to change notification settings - Fork 37
Rewrite codebase in TypeScript for v3.0.0 - Clean types with documented interfaces and strict ESLint rules #45
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
base: master
Are you sure you want to change the base?
Conversation
…patterns Co-authored-by: albertorestifo <[email protected]>
|
@copilot I think we can remove the JSDoc comments from the code now that we have types. Please rewiew all types and singature of the functions, we should take advantage of typescript to build a solid type system for the graph sturcture. Simplify the documentation to also simply reference the types, there is no need to be so verbose anymore now that it's strictly type. Do not mention older releases of this package anymore in the readme, there is no need really. Add a pipeline to make sure it works with node latest as well. |
…de.js latest CI Co-authored-by: albertorestifo <[email protected]>
Done! I've implemented all your requested changes: ✅ Removed JSDoc comments - TypeScript types now provide the documentation Additional improvements:
All 56 tests passing with full type safety! The codebase is now much cleaner and leverages TypeScript's type system effectively. Commit: bebaa8b |
.eslintrc
Outdated
| ], | ||
| "rules": { | ||
| "@typescript-eslint/no-unused-vars": ["error", { "argsIgnorePattern": "^_" }], | ||
| "no-dupe-class-members": "off" |
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.
@copilot Remove this exceptions to the rules. We want vanilla eslint:recommented and typescript strict rules.
After changing this, make sure everything passes.
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.
Removed all ESLint rule exceptions as requested. Now using vanilla eslint:recommended plus strict TypeScript rules:
@typescript-eslint/no-unused-vars@typescript-eslint/no-explicit-any@typescript-eslint/no-var-requires
Had to refactor method overloads to avoid no-dupe-class-members errors while maintaining type safety. All 56 tests passing with strict rules.
Commit: cdacdfa
…t strict rules Co-authored-by: albertorestifo <[email protected]>
src/toDeepMap.ts
Outdated
| return !isNaN(cost) && cost > 0; | ||
| } | ||
|
|
||
| export type GraphNode = number | Map<string | number, GraphNode>; |
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.
@copilot this should re-use types defined in Graph.ts and be moved there.
Also the types in the other palces might need to use this type for clarity.
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.
Moved GraphNode type definition to Graph.ts and updated all modules to import types from the centralized location. Now all type definitions (NodeKey, EdgeWeight, GraphNode) are consistently managed in Graph.ts for better clarity and maintainability.
Updated imports in:
toDeepMap.tsvalidateDeep.tsremoveDeepFromMap.ts
All 56 tests passing with the consolidated type system.
Commit: 4a7ad91
…mports for better type clarity Co-authored-by: albertorestifo <[email protected]>
Co-authored-by: albertorestifo <[email protected]>
|
@copilot Solve the conflicts with master, and remove the .map JS files from the repo, why are they even needed? |
Co-authored-by: albertorestifo <[email protected]>
Resolved conflicts with master by keeping v3.0.0 changes (package.json and README.md). Merged in the release workflow and CHANGELOG from master. Removed all .map files (JavaScript and TypeScript source maps) from the repository. These files were generated by the TypeScript compiler during build ( Added All 56 tests passing after the merge. |
|
@copilot In the CI, test the following (oldest LTS supported releases + latest): 20, 26, latest |
Co-authored-by: albertorestifo <[email protected]>
|
@copilot my bad, remove node26 from CI |
Co-authored-by: albertorestifo <[email protected]>
TypeScript Migration Complete - node-dijkstra v3.0.0
Successfully Completed ✅
Latest Improvements ✅
NodeKey,EdgeWeight,GraphNode)addVertex,shortestPath) for cleaner APIESLint Strict Configuration ✅
anyusage when testing private membersType System Consolidation ✅
NodeKey,EdgeWeight,GraphNodeInterface Documentation ✅
trim: Exclude the origin and destination nodes from the resultreverse: Return the path in reversed order, from goal to startcost: Return an object with path and cost instead of just the pathavoid: Array of nodes to be avoided when finding the pathRepository Cleanup ✅
*.mapfiles going forwardCI/CD Configuration ✅
Current ESLint Rules
eslint:recommended(vanilla)@typescript-eslint/no-unused-vars@typescript-eslint/no-explicit-any@typescript-eslint/no-var-requiresAll 56 tests passing with strict TypeScript rules, consolidated type system, documented interfaces, clean repository, and modern CI/CD pipeline!
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.