Fix Node SDK strict TypeScript consumer compatibility - #2639
Closed
patschmittdev wants to merge 1 commit into
Closed
Fix Node SDK strict TypeScript consumer compatibility#2639patschmittdev wants to merge 1 commit into
patschmittdev wants to merge 1 commit into
Conversation
Preserve lifecycle event metadata variants while migrating JSON-RPC to its supported Node export. Add strict packed public consumer checks and deterministic compatibility coverage for JSON-RPC 8 peers. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The samples lockfile is inconsistent with JSON-RPC 9, and the strict consumer verifier is not run by CI.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates the Node SDK for strict TypeScript consumer compatibility and migrates runtime JSON-RPC usage to vscode-jsonrpc 9.
Changes:
- Refines lifecycle typings and enables export-aware resolution.
- Updates JSON-RPC imports, dependencies, and compatibility coverage.
- Adds strict packed-consumer fixtures, verification tooling, and documentation.
- Follow-ups required: regenerate the samples lockfile and run consumer verification in CI.
File summaries
| File | Summary |
|---|---|
scripts/codegen/typescript.ts |
Updates generated RPC imports. |
nodejs/tsconfig.test.json |
Includes compatibility test compilation. |
nodejs/tsconfig.json |
Enables export-aware resolution. |
nodejs/test/session-send-and-wait.test.ts |
Updates RPC imports. |
nodejs/test/message-source.test.ts |
Updates RPC imports. |
nodejs/test/jsonrpc-compat.test.ts |
Adds JSON-RPC 8/9 interoperability coverage. |
nodejs/test/factory.test.ts |
Updates RPC imports. |
nodejs/test/e2e/extension_env_access.e2e.test.ts |
Updates RPC imports. |
nodejs/test/consumer-types/tsconfig.json |
Configures strict consumer compilation. |
nodejs/test/consumer-types/tsconfig.consumer.json |
Configures declaration-consumer checks. |
nodejs/test/consumer-types/runtime-extension.mjs |
Tests packed extension runtime behavior. |
nodejs/test/consumer-types/lifecycle.mts |
Tests lifecycle narrowing and metadata requirements. |
nodejs/test/consumer-types/extension.mts |
Tests extension typings. |
nodejs/test/consumer-types/consumer.mts |
Tests emitted declaration reuse. |
nodejs/test/client.test.ts |
Updates dynamic RPC imports. |
nodejs/src/types.ts |
Fixes lifecycle event base typing. |
nodejs/src/session.ts |
Uses the JSON-RPC 9 node export. |
nodejs/src/generated/rpc.ts |
Regenerates the RPC import. |
nodejs/src/client.ts |
Updates JSON-RPC imports. |
nodejs/scripts/verify-consumer-types.mjs |
Adds packed-package verification. |
nodejs/samples/package-lock.json |
Contains inconsistent JSON-RPC resolution metadata requiring regeneration. |
nodejs/README.md |
Documents strict consumer validation. |
nodejs/package.json |
Updates the dependency and verifier script. |
nodejs/package-lock.json |
Locks JSON-RPC 9 and compatibility coverage. |
Review details
Files not reviewed (2)
- nodejs/package-lock.json: Generated file
- nodejs/samples/package-lock.json: Generated file
Suppressed comments (1)
nodejs/package.json:43
- The strict packed-consumer verifier is exposed as a script but is not invoked by
.github/workflows/nodejs-sdk-tests.yml: CI runstypecheck, build/package verification, and the Vitest suite, but nevertest:consumer-types. The compatibility regression this PR targets can therefore return without failing CI; add a validation step that packs the SDK and runs this verifier (at least for one supported Node/types pair).
"test:consumer-types": "node scripts/verify-consumer-types.mjs",
- Files reviewed: 21/24 changed files
- Comments generated: 1
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| "dependencies": { | ||
| "koffi": "^3.2.1", | ||
| "vscode-jsonrpc": "^8.2.1", | ||
| "vscode-jsonrpc": "^9.0.2", |
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.
Summary
Fix strict public-package consumption without weakening lifecycle event variants or using
skipLibCheckor a dependency override.metadataonly from the privateSessionLifecycleEventBase. All five public variants remain unchanged: created/updated/foreground/background require metadata, and deleted keepsmetadata?: undefined, matching the runtime's own property with an undefined value.vscode-jsonrpcdependency from 8.2.1 to 9.0.2 and consistently use its supportedvscode-jsonrpc/nodeexport. Update the TypeScript generator and regenerate against the current CLI pin; the generated diff is only that import. Preserve current RPC, session-event, message-source, and FFI changes.Package version
0.0.0-dev, CLI pin1.0.84-5, andkoffirequirement^3.2.1are unchanged. No release, tag, or package publication is part of this PR. The changelog is release-generated, so no release entry or version bump is added.Context and reproduction
The lifecycle union originated in #1357. Related historical type/export reports are #133 and #891; these references provide context, not requests to reopen or close those items.
A public SDK consumer with
strict: true,exactOptionalPropertyTypes: true, andskipLibCheck: falseencounters TS2430 becauseSessionDeletedEvent.metadata?: undefinedcannot extend the old private base'smetadata?: SessionLifecycleEventMetadata.The packed consumer now compiles. Restoring only the old private-base property inside an isolated installed declaration reproduces TS2430, demonstrating regression sensitivity. Fixtures also assert discriminant narrowing, required metadata for the four non-deleted variants, deleted metadata rejection, public extension inference, and emitted declaration reuse.
Compatibility considerations
This is an explicit JSON-RPC major dependency migration, not a blanket drop-in compatibility claim. Consumers need export-aware module resolution (
NodeNext,Node16, orbundler), and JSON-RPC 9 declarations require TypeScript 5.4 or newer. PublicMessageConnectiontyping follows JSON-RPC 9. Existing applications using older compiler/resolver configurations or JSON-RPC 8-specific types may require changes.Bidirectional JSON-RPC 8/9 requests, notifications, structured errors, cancellation, and default async-notification scheduling are covered. Wire interoperability does not establish TypeScript source compatibility for every previous setup.
Validation
Performed on this current-main-based shipping change:
@types/node22.20.2/24.13.4 passed isolated packed-consumer checks using TypeScript 7.0.2: strict declarations, second declaration consumer, public ESM/CJS exports, and a real child extension against a deterministic JSON-RPC 8 peer.Intended target:
github/copilot-sdk:main; head:patschmittdev/copilot-sdk:patschmittdev-ship-sdk-strict-types.