v2.0.0: TypeScript rewrite with full v1 compatibility#36
Merged
Conversation
Complete rewrite of the library in strict TypeScript (Node >= 18.17), keeping the entire v1 API working through a deprecated compat layer. Highlights: - Zero runtime dependencies: own CRC-16/X-25 replaces crc, crc16-ccitt-node, node.extend and the undeclared crc-itu (#25, #30) - Proper TCP framing: fragmented/coalesced packets are reassembled (v1 assumed 1 socket event = 1 packet) - Socket errors no longer crash the process (#12); idle connections are destroyed after a configurable timeout (#31) - New protocols: SinoTrack ST-901 / H02 (#19) and Concox GK309 (#26), which also fixes the wrong CRC variant used for GT06 acks - Typed events, dual ESM/CJS build (tsup), vitest suite (61 tests with fixtures from official protocol docs + real TCP integration tests), ESLint, CI matrix for Node 18/20/22 and npm publish with provenance - v1 API (gps.server, snake_case events, custom v1 adapters) still works unchanged; using it emits one DeprecationWarning per process. See MIGRATION.md for the mapping and behaviour notes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KUhDLb7eGUBraFsnhBoRGm
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (13)
📝 WalkthroughWalkthroughChangesThis change introduces a TypeScript v2 GPS tracking server with TCP framing, typed events, five built-in protocol adapters, a deprecated v1 compatibility layer, ESM/CJS packaging, migration documentation, CI/release workflows, and unit and integration tests. GPS tracking server v2
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant GPSClient
participant GpsServer
participant Device
participant Adapter
GPSClient->>GpsServer: Open TCP connection
GpsServer->>Device: Create device connection
GPSClient->>Device: Send protocol frame
Device->>Adapter: Parse frame
Adapter-->>Device: Return parsed packet
Device-->>GpsServer: Emit device or packet event
Adapter->>GPSClient: Send login or packet acknowledgement
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (1)
.github/workflows/release.yml (1)
20-25: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid running the release checks twice.
package.jsondefinesprepublishOnlyto run typecheck, lint, build, and tests;npm publishinvokes that lifecycle hook. Lines 21–24 therefore repeat the same work before publishing. Remove the explicit duplicates or narrowprepublishOnly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml around lines 20 - 25, Update the release workflow steps surrounding npm publish to remove the explicit typecheck, lint, build, and test commands, since npm publish invokes package.json’s prepublishOnly hook. Keep npm ci and the publish command unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 15: Update the actions/checkout@v4 step in the CI workflow to set
persist-credentials to false, preventing the checkout token from remaining in
local Git configuration before later npm scripts execute.
In @.github/workflows/release.yml:
- Line 14: Update the actions/checkout@v4 step in the release workflow to set
persist-credentials to false, ensuring the checkout does not retain the GitHub
token in local Git configuration while preserving the existing checkout
behavior.
In `@eslint.config.js`:
- Around line 11-12: Update the ESLint configuration’s tsconfigRootDir setting
to use a Node 18-compatible directory lookup via fileURLToPath(new URL('.',
import.meta.url)), adding the required URL-to-path import if needed, and
preserve projectService: true.
In `@examples/simple.ts`:
- Line 3: Update the run instruction comment in examples/simple.ts to remove the
plain tsc compilation note, leaving only the npx tsx command or replacing it
with a verified emitting build command.
In `@package.json`:
- Around line 62-65: Align the eslint toolchain with the project’s advertised
Node 18.17 minimum by downgrading both eslint and `@eslint/js` from version 10 to
compatible versions. Keep the package runtime metadata and CI minimum unchanged
unless intentionally raising them together.
In `@README.md`:
- Around line 7-8: Update the README project description to state Node.js
>=18.17 instead of Node.js >=18, matching the minimum version required by
package.json and MIGRATION.md.
In `@src/adapters/tk103.ts`:
- Around line 93-116: Update `#parsePosition` to require the complete course field
before constructing the GpsPosition: increase the minimum payload-length
validation to cover the data.slice(39, 45) range, while preserving the existing
coordinate and position parsing behavior.
In `@src/adapters/tk510.ts`:
- Around line 74-76: In the `9999` case, validate that `data.length` is at least
one before calling `data.readUInt8(0)`, and throw the adapter’s
`PacketParseError` for an empty payload so corrupt packets follow the
established contract.
In `@src/compat.ts`:
- Around line 112-146: Preserve the parsed login packet separately in the
frame-handling logic around `#lastParts` and the login_request case, rather than
allowing subsequent frames to replace it. Update authorize() to pass the stored
login packet to `#instance.authorize`, while retaining the existing parsing
behavior for other packet types.
In `@src/framing/delimiter-framer.ts`:
- Around line 42-50: Update the delimiter-processing loop around the buffer
length check and frames.push so completed frames are also limited by
`#maxFrameLength`: when the segment through the detected end delimiter exceeds the
cap, discard it instead of emitting it. Preserve normal emission and buffer
advancement for frames within the limit.
In `@src/lib/geo.ts`:
- Around line 16-21: Update minuteToDecimal to validate the parsed minutes
component before returning a coordinate: reject values where minutes are outside
the valid NMEA range of 0–59, including malformed values such as 1260.000, by
returning the existing invalid-coordinate representation expected by downstream
NaN checks. Preserve the hemisphere sign handling for valid inputs.
In `@src/lib/protocol.ts`:
- Around line 17-22: Update digits2 and the timestamp parsing flow to reject
malformed two-character fields instead of accepting partial parseInt results,
and validate all parsed date/time components before constructing the Date.
Ensure invalid or overflowing values, including out-of-range months and
normalized dates, are rejected rather than passed to Date.UTC.
In `@src/server.ts`:
- Around line 113-115: Update the device event handling in the server connection
setup so the `identified` listener does not add sockets to `#devices`; keep
those sockets tracked only in `#connections`. Register the device in `#devices`
from its authenticated `login` event, after `acceptLogin()` succeeds, preserving
`sendTo()` routing exclusively for authenticated connections.
In `@test/integration/compat.test.ts`:
- Around line 75-82: Update the test around startLegacyServer to install the
process.emitWarning spy before the first legacy-server creation in the worker,
or isolate the module so the warning is observable. Change the final assertion
to require exactly one DeprecationWarning call while preserving the two
server-creation checks.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 20-25: Update the release workflow steps surrounding npm publish
to remove the explicit typecheck, lint, build, and test commands, since npm
publish invokes package.json’s prepublishOnly hook. Keep npm ci and the publish
command unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 64b9b455-912b-4e4f-b270-bd9aa31b94d1
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (53)
.github/workflows/ci.yml.github/workflows/release.yml.gitignore.jscsrcMIGRATION.mdREADME.mdeslint.config.jsexamples/legacy-v1.cjsexamples/simple.jsexamples/simple.tsindex.jslib/adapters/gt02a.jslib/adapters/gt06.jslib/adapters/tk103.jslib/adapters/tk510.jslib/device.jslib/functions.jslib/server.jspackage.jsonreadme.mdsrc/adapters/base-adapter.tssrc/adapters/gt02a.tssrc/adapters/gt06.tssrc/adapters/index.tssrc/adapters/st901.tssrc/adapters/tk103.tssrc/adapters/tk510.tssrc/compat.tssrc/device.tssrc/errors.tssrc/framing/delimiter-framer.tssrc/framing/framer.tssrc/framing/length-prefixed-framer.tssrc/index.tssrc/lib/crc.tssrc/lib/geo.tssrc/lib/protocol.tssrc/server.tssrc/typed-emitter.tssrc/types.tstest/helpers/fake-device.tstest/helpers/tcp.tstest/integration/compat.test.tstest/integration/server.test.tstest/unit/crc.test.tstest/unit/framers.test.tstest/unit/geo.test.tstest/unit/gt02a.test.tstest/unit/gt06.test.tstest/unit/st901.test.tstest/unit/tk103.test.tstest/unit/tk510.test.tstsconfig.json
💤 Files with no reviewable changes (11)
- index.js
- examples/simple.js
- lib/adapters/tk510.js
- readme.md
- lib/device.js
- .jscsrc
- lib/adapters/tk103.js
- lib/adapters/gt02a.js
- lib/functions.js
- lib/server.js
- lib/adapters/gt06.js
Comment on lines
+62
to
+65
| "devDependencies": { | ||
| "@eslint/js": "^10.0.1", | ||
| "@types/node": "^18.19.0", | ||
| "eslint": "^10.7.0", |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
npm view eslint@10 engines --json
npm view `@eslint/js`@10 engines --jsonRepository: freshworkstudio/gps-tracking-nodejs
Length of output: 1260
🏁 Script executed:
#!/bin/sh
set -eu
printf '\n== package.json ==\n'
cat -n package.json | sed -n '1,140p'
printf '\n== package-lock candidates ==\n'
git ls-files 'package-lock.json' 'npm-shrinkwrap.json' || true
printf '\n== eslint package metadata ==\n'
npm view eslint@10 version engines --json
printf '\n== `@eslint/js` package metadata ==\n'
npm view `@eslint/js`@10 version engines --jsonRepository: freshworkstudio/gps-tracking-nodejs
Length of output: 4382
🏁 Script executed:
#!/bin/sh
set -eu
python3 - <<'PY'
import json, subprocess
for pkg in ["eslint@10", "`@eslint/js`@10"]:
out = subprocess.check_output(["npm","view",pkg,"version","engines","--json"], text=True)
print(f"\n## {pkg}\n{out}")
PYRepository: freshworkstudio/gps-tracking-nodejs
Length of output: 2044
🏁 Script executed:
#!/bin/sh
set -eu
# Show just the dependency declarations around the reported lines.
sed -n '55,75p' package.jsonRepository: freshworkstudio/gps-tracking-nodejs
Length of output: 654
Keep the lint toolchain aligned with the Node 18.17 floor. eslint@10 requires Node >=20.19.0, and @eslint/js@10.0.1 matches that requirement, so lint/prepublishOnly won’t work on the advertised runtime range. Downgrade both to a Node-18-compatible line or raise the package and CI minimum together.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@package.json` around lines 62 - 65, Align the eslint toolchain with the
project’s advertised Node 18.17 minimum by downgrading both eslint and
`@eslint/js` from version 10 to compatible versions. Keep the package runtime
metadata and CI minimum unchanged unless intentionally raising them together.
- CI/release: set persist-credentials: false on checkout (zizmor artipacked) - CI: run ESLint in a separate Node 22 job (eslint@10 needs Node >= 20.19, above the package's 18.17 runtime floor) - eslint.config: Node 18-compatible dirname via fileURLToPath - Server: register devices in the routable map only after login, so an unauthenticated peer cannot hijack an existing id via getDevice()/sendTo() - DelimiterFramer: drop oversized frames even when properly terminated - minuteToDecimal/utcDate/digits2: reject malformed coordinates and overflowing timestamps instead of emitting bogus fixes - TK103: require the course field in position payloads (min length 45) - TK510: explicit PacketParseError for empty alarm payloads - Compat: preserve the pending login parts for authorize() even if other frames are parsed in between - Tests: assert exactly one DeprecationWarning on a fresh module registry; regression tests for the framer cap and coordinate validation - README/examples: Node >= 18.17 wording, drop the tsc compile note Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KUhDLb7eGUBraFsnhBoRGm
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
1 participant
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.
Summary
Complete rewrite of the library in strict TypeScript (Node >= 18.17), keeping the entire v1 API working unchanged through a deprecated compatibility layer. Existing v1 code runs as-is; the only hard break is the Node version requirement.
Fixes long-standing issues
ECONNRESET) no longer crash the processcrc,crc16-ccitt-node,node.extendand the undeclaredcrc-itu)What's inside
loginRequest,ping,alarm,parseError, ...) with aGpsPositionpayloadMIGRATION.mdwith the full v1→v2 mapping and behaviour notesCompatibility
gps.server(options, callback), snake_case events,login_authorized(true), string adapter names and custom v1 adapter modules all keep working, marked@deprecated(oneDeprecationWarningper process). Documented behaviour changes (GT06 ids without the leading BCD zero, TK510 dates read as DDMMYY, UTC timestamps) are listed in MIGRATION.md.Release plan
Publish as
2.0.0-beta.1(npm publish --tag beta) before the final2.0.0. Requires theNPM_TOKENsecret for the release workflow.🤖 Generated with Claude Code
https://claude.ai/code/session_01KUhDLb7eGUBraFsnhBoRGm
Summary by CodeRabbit