feat(client): support ORPCError instanceof checks across different dependency graph#972
Conversation
dosubot
Bot
added
the
size:L
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds build-time placeholders for the client package, exports those placeholder constants, implements a global registry and a Symbol.hasInstance override on ORPCError for cross-context instanceof checks, and introduces a shared getConstructor helper with tests covering null-proto and function cases. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as User Code
participant Inst as Error Instance
participant ORPC as ORPCError Class
participant REG as Global Registry (Symbol.for "orpc:<name>@<version>")
participant GET as getConstructor (shared)
User->>ORPC: inst instanceof ORPCError?
activate ORPC
ORPC->>REG: ensure/read registry contains constructors
ORPC->>GET: getConstructor(inst)
GET-->>ORPC: ctorInst (Function|null|undefined)
alt ctorInst && ctorInst in REG && ORPC in REG
ORPC-->>User: true
else
ORPC->>ORPC: delegate to super[Symbol.hasInstance](instance)
ORPC-->>User: boolean
end
deactivate ORPC
note right of REG: Registry persists on globalThis across contexts to enable cross-context instanceof
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Assessment — Out-of-scope changes
Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @unnoq, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request provides a robust solution for ORPCError instanceof checks failing in environments with multiple JavaScript dependency graphs, such as Next.js's Optimized SSR. By globally registering ORPCError constructors and overriding the Symbol.hasInstance method, it ensures consistent and reliable error type identification across different execution contexts, improving the stability of error handling.
Highlights
- Cross-context instanceof for ORPCError: Implemented a workaround to enable instanceof ORPCError checks to function correctly across different JavaScript contexts, specifically addressing issues in environments like Next.js where separate dependency graphs can lead to multiple ORPCError constructors.
- Global Constructor Registry: Utilized Symbol.for to create a global registry for ORPCError constructors, allowing the ORPCError class to identify instances created by different constructors from other contexts.
- Symbol.hasInstance Override: Overrode the Symbol.hasInstance method on the ORPCError class to leverage the global constructor registry, ensuring that instanceof checks correctly identify ORPCError instances regardless of their originating context.
- New Utility Function: Introduced a getConstructor utility function in @orpc/shared to reliably retrieve the constructor of an object, which is crucial for the new instanceof workaround.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces an intelligent workaround to address instanceof ORPCError check failures in environments with multiple dependency graphs, such as Next.js. The approach of using a global symbol to share ORPCError constructors and overriding Symbol.hasInstance is well-implemented and robust. The changes are well-tested, including a new helper function getConstructor. Overall, this is a high-quality contribution. I have a couple of minor suggestions to improve the inline comments for better maintainability of the new logic.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
More templates
@orpc/arktype
@orpc/client
@orpc/contract
@orpc/experimental-durable-event-iterator
@orpc/hey-api
@orpc/interop
@orpc/json-schema
@orpc/nest
@orpc/openapi
@orpc/openapi-client
@orpc/otel
@orpc/react
@orpc/react-query
@orpc/experimental-react-swr
@orpc/server
@orpc/shared
@orpc/solid-query
@orpc/standard-server
@orpc/standard-server-aws-lambda
@orpc/standard-server-fetch
@orpc/standard-server-node
@orpc/standard-server-peer
@orpc/svelte-query
@orpc/tanstack-query
@orpc/trpc
@orpc/valibot
@orpc/vue-colada
@orpc/vue-query
@orpc/zod
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
packages/client/build.config.ts (1)
4-9: Add CI/local check to verify placeholder replacement
Include a step that installs dependencies, runs the client build (unbuild), and fails if any__ORPC_CLIENT_PACKAGE_NAME_PLACEHOLDER__or__ORPC_CLIENT_PACKAGE_VERSION_PLACEHOLDER__remain inpackages/client/dist. Example:pnpm install pnpm -w --filter @orpc/client run build rg -nP '__ORPC_CLIENT_PACKAGE_(NAME|VERSION)_PLACEHOLDER__' packages/client/dist && { echo "Found unreplaced placeholders"; exit 1; } || { echo "OK: no placeholders found"; }packages/client/src/consts.ts (1)
1-2: Nit: mark as const type to help inliningNot required, but asserting literal types can aid DCE/minification.
-export const ORPC_CLIENT_PACKAGE_NAME = '__ORPC_CLIENT_PACKAGE_NAME_PLACEHOLDER__' -export const ORPC_CLIENT_PACKAGE_VERSION = '__ORPC_CLIENT_PACKAGE_VERSION_PLACEHOLDER__' +export const ORPC_CLIENT_PACKAGE_NAME = '__ORPC_CLIENT_PACKAGE_NAME_PLACEHOLDER__' as const +export const ORPC_CLIENT_PACKAGE_VERSION = '__ORPC_CLIENT_PACKAGE_VERSION_PLACEHOLDER__' as constpackages/shared/src/object.ts (1)
30-40: Type the return as Function to reflect constructorsgetConstructor returns prototype constructors (including classes), which align better with Function than AnyFunction (call signature). Optional, but removes type friction down the line.
-import type { AnyFunction } from './function' ... -export function getConstructor(value: unknown): AnyFunction | null | undefined { +export function getConstructor(value: unknown): Function | null | undefined {packages/client/src/error.test.ts (1)
30-63: Add a cross-graph simulation test to prove the workaroundCurrent tests validate normal instanceof only. Simulate a second dependency graph by registering a fake constructor via the shared symbol and assert ORPCError recognition.
@@ it('instanceof should behave as normal', () => { @@ }) + + it('instanceof should recognize errors from another dependency graph', () => { + class FakeORPCError extends Error {} + // Compute the same global symbol used by the library + const { ORPC_CLIENT_PACKAGE_NAME, ORPC_CLIENT_PACKAGE_VERSION } = require('./consts') + const sym = Symbol.for(`__${ORPC_CLIENT_PACKAGE_NAME}@${ORPC_CLIENT_PACKAGE_VERSION}/error/ORPCErrorConstructors__`) + const store: unknown[] = (globalThis as any)[sym] + try { + store.push(FakeORPCError) + const foreign = new FakeORPCError('x') + expect(foreign instanceof ORPCError).toBe(true) + } finally { + const i = store.indexOf(FakeORPCError) + if (i >= 0) store.splice(i, 1) + } + })packages/client/src/error.ts (1)
157-167: Tighten instanceof guard and short-circuit for non-objectsMinor: getConstructor returns null for primitives; early-return avoids touching the set. Also switch to WeakSet.has when applying the refactor above.
- static override[Symbol.hasInstance](instance: unknown): boolean { - if ( - GlobalORPCErrorConstructors.includes(this) // not applicable to extended classes - && GlobalORPCErrorConstructors.includes(getConstructor(instance)) - ) { + static override [Symbol.hasInstance](instance: unknown): boolean { + const ctor = getConstructor(instance) + if ( + GlobalORPCErrorConstructors.has(this as unknown as Function) // not applicable to extended classes + && ctor + && GlobalORPCErrorConstructors.has(ctor) + ) { return true } // fallback to default instanceof check if this is extended from ORPCError return super[Symbol.hasInstance](instance) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/client/build.config.ts(1 hunks)packages/client/src/consts.ts(1 hunks)packages/client/src/error.test.ts(2 hunks)packages/client/src/error.ts(3 hunks)packages/client/src/index.ts(1 hunks)packages/shared/src/object.test.ts(2 hunks)packages/shared/src/object.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
packages/shared/src/object.ts (1)
packages/shared/src/function.ts (1)
AnyFunction(1-1)
packages/shared/src/object.test.ts (1)
packages/shared/src/object.ts (2)
getConstructor(33-40)NullProtoObj(99-104)
packages/client/src/error.test.ts (2)
packages/client/src/error.ts (1)
ORPCError(112-168)packages/shared/src/object.ts (1)
NullProtoObj(99-104)
packages/client/src/error.ts (2)
packages/client/src/consts.ts (2)
ORPC_CLIENT_PACKAGE_NAME(1-1)ORPC_CLIENT_PACKAGE_VERSION(2-2)packages/shared/src/object.ts (1)
getConstructor(33-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: publish-commit
- GitHub Check: lint
🔇 Additional comments (5)
packages/client/src/index.ts (1)
3-3: Publicly re-exporting consts is reasonableMakes the replacement key computable in tests/consumers. No issues spotted.
packages/shared/src/object.test.ts (1)
36-45: Good coverage of edge-cases for getConstructorNull/undefined/primitives, Error, null-proto, and function cases covered. Looks solid.
packages/shared/src/object.ts (1)
45-53: isObject heuristic is intentionally narrowCalling out: Map/Set/Date deliberately excluded. No change requested.
packages/client/src/error.test.ts (1)
27-28: Nice sanity checkConstructor name assertion is useful alongside instanceof assertions.
packages/client/src/error.ts (1)
177-179: Semantics of isDefinedError remain correct with custom hasInstanceGood to see predicate relies on instanceof; your Symbol.hasInstance override keeps behavior consistent across graphs.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/client/src/error.ts (2)
145-167: Strengthen cross-context instanceof to cover subclasses; tighten comments; swap to WeakSet.has.Right now only base ORPCError instances from another graph pass; subclass instances won’t (constructor differs and isn’t registered). Walking the prototype chain of the instance’s constructor fixes that and keeps the behavior scoped to the base class. Also adopt WeakSet.has when you switch the registry.
Apply:
- static override[Symbol.hasInstance](instance: unknown): boolean { - if ( - GlobalORPCErrorConstructors.includes(this) // not applicable to extended classes - && GlobalORPCErrorConstructors.includes(getConstructor(instance)) - ) { - return true - } - - // fallback to default instanceof check - return super[Symbol.hasInstance](instance) - } + static override[Symbol.hasInstance](instance: unknown): boolean { + // Apply custom logic only when RHS is a registered base ORPCError constructor. + if (GlobalORPCErrorConstructors.has(this as any)) { + // Support subclasses across contexts by walking the constructor chain. + let ctor = getConstructor(instance) + while (ctor) { + if (GlobalORPCErrorConstructors.has(ctor)) return true + const proto = (ctor as any)?.prototype && Object.getPrototypeOf((ctor as any).prototype) + ctor = proto?.constructor + } + } + // Fallback to the default prototype-chain-based instanceof check. + return super[Symbol.hasInstance](instance) + }This also addresses prior feedback about the inline comments being imprecise.
169-173: Make registration idempotent.Avoid duplicates across HMR by using WeakSet.add (or guard push if you keep Array).
Apply one of:
- With WeakSet:
-GlobalORPCErrorConstructors.push(ORPCError) +GlobalORPCErrorConstructors.add(ORPCError)
- If you keep Array:
-if (true) GlobalORPCErrorConstructors.push(ORPCError) +if (!GlobalORPCErrorConstructors.includes(ORPCError)) { + GlobalORPCErrorConstructors.push(ORPCError) +}
🧹 Nitpick comments (1)
packages/client/src/error.ts (1)
102-111: Use WeakSet for the global registry; avoid duplicates, O(1) lookups, and dev-time leaks.Array.includes is O(n), allows duplicates across HMR, and keeps strong refs. WeakSet dedups and lets GC reclaim unused constructors.
Apply:
const GlobalORPCErrorConstructorsSymbol = Symbol.for(`__${ORPC_CLIENT_PACKAGE_NAME}@${ORPC_CLIENT_PACKAGE_VERSION}/error/ORPCErrorConstructors__`) -void ((globalThis as any)[GlobalORPCErrorConstructorsSymbol] ??= []) -const GlobalORPCErrorConstructors: unknown[] = (globalThis as any)[GlobalORPCErrorConstructorsSymbol] +(globalThis as any)[GlobalORPCErrorConstructorsSymbol] ||= new WeakSet<Function>() +const GlobalORPCErrorConstructors: WeakSet<Function> = (globalThis as any)[GlobalORPCErrorConstructorsSymbol]Nit: dropping the leading
voidsimplifies intent without changing semantics:-void ((globalThis as any)[GlobalORPCErrorConstructorsSymbol] ??= []) +(globalThis as any)[GlobalORPCErrorConstructorsSymbol] ||= new WeakSet<Function>()Note: This approach assumes both module graphs share the same globalThis (true for Next.js Optimized SSR, not for Edge/Workers). Please confirm your target environments share globals; otherwise this workaround won’t bridge isolates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/client/src/error.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/client/src/error.ts (2)
packages/client/src/consts.ts (2)
ORPC_CLIENT_PACKAGE_NAME(1-1)ORPC_CLIENT_PACKAGE_VERSION(2-2)packages/shared/src/object.ts (1)
getConstructor(33-40)
🔇 Additional comments (1)
packages/client/src/error.ts (1)
2-3: Imports and build-time const usage look good.getConstructor is correctly consumed; package name/version placeholders align with the Symbol.for keying below.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/client/src/error.ts (1)
158-158: Clarify the intent of the subclass gating commentMake the comment explicit about base-class-only behavior to aid future readers.
Apply:
- // not applicable to extended classes + // Apply custom logic only for the base ORPCError constructor; subclasses fall back to default semantics.
🧹 Nitpick comments (2)
packages/client/src/error.ts (2)
108-111: Type the global registry as WeakSet and parametrize the initializerThese entries are constructor functions. Tightening the type improves readability and tooling, and avoids accidental non-callable entries.
Apply:
-const GLOBAL_ORPC_ERROR_CONSTRUCTORS_SYMBOL = Symbol.for(`__${ORPC_CLIENT_PACKAGE_NAME}@${ORPC_CLIENT_PACKAGE_VERSION}/error/ORPC_ERROR_CONSTRUCTORS__`) -void ((globalThis as any)[GLOBAL_ORPC_ERROR_CONSTRUCTORS_SYMBOL] ??= new WeakSet()) -const globalORPCErrorConstructors: WeakSet<object> = (globalThis as any)[GLOBAL_ORPC_ERROR_CONSTRUCTORS_SYMBOL] +const GLOBAL_ORPC_ERROR_CONSTRUCTORS_SYMBOL = Symbol.for(`__${ORPC_CLIENT_PACKAGE_NAME}@${ORPC_CLIENT_PACKAGE_VERSION}/error/ORPC_ERROR_CONSTRUCTORS__`) +void ((globalThis as any)[GLOBAL_ORPC_ERROR_CONSTRUCTORS_SYMBOL] ??= new WeakSet<Function>()) +const globalORPCErrorConstructors: WeakSet<Function> = (globalThis as any)[GLOBAL_ORPC_ERROR_CONSTRUCTORS_SYMBOL]
108-108: Optional: make the global key version-agnostic to survive patch bumpsUsing the exact version in the key can split the registry after minor/patch updates in mixed graphs, reintroducing instanceof issues. If you want cross-graph compatibility across same-major versions (or any version), drop the version (or encode only major).
Minimal option (versionless):
-const GLOBAL_ORPC_ERROR_CONSTRUCTORS_SYMBOL = Symbol.for(`__${ORPC_CLIENT_PACKAGE_NAME}@${ORPC_CLIENT_PACKAGE_VERSION}/error/ORPC_ERROR_CONSTRUCTORS__`) +const GLOBAL_ORPC_ERROR_CONSTRUCTORS_SYMBOL = Symbol.for(`__${ORPC_CLIENT_PACKAGE_NAME}__/error/ORPC_ERROR_CONSTRUCTORS__`)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/client/src/error.ts(3 hunks)packages/shared/src/object.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/shared/src/object.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/client/src/error.ts (2)
packages/client/src/consts.ts (2)
ORPC_CLIENT_PACKAGE_NAME(1-1)ORPC_CLIENT_PACKAGE_VERSION(2-2)packages/shared/src/object.ts (1)
getConstructor(32-39)
🪛 Biome (2.1.2)
packages/client/src/error.ts
[error] 160-160: Do not shadow the global "constructor" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: publish-commit
- GitHub Check: test
- GitHub Check: lint
🔇 Additional comments (3)
packages/client/src/error.ts (3)
2-3: LGTM: Imports align with new cross-context logicThe added imports are correct and used appropriately in this file.
157-168: Symbol.hasInstance fallback and gating logic look solidCustom logic only runs when checking against the base class; otherwise it defers to default prototype-chain semantics. This preserves subclass behavior.
170-174: Idempotent global registration is correctUsing WeakSet.add ensures no duplicates across HMR and avoids leaks. Good call.
1 participant
Workaround for Next.js where different contexts use separate
dependency graphs, causing multiple ORPCError constructors existing and breaking
instanceofchecks across contexts.This is particularly problematic with "Optimized SSR", where orpc-client
executes in one context but is invoked from another. When an error is thrown
in the execution context,
instanceof ORPCErrorchecks fail in theinvocation context due to separate class constructors.
Closes: https://github.com/unnoq/orpc/issues/957
Summary by CodeRabbit