feat!: update standard server #179
Conversation
WalkthroughThe changes primarily refactor import statements across multiple packages by shifting many imports from Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant OnceWrapper
participant OriginalFn
Caller->>OnceWrapper: Call once-wrapped function
alt First Call
OnceWrapper->>OriginalFn: Execute Original Function
OriginalFn-->>OnceWrapper: Return result
OnceWrapper-->>Caller: Return and cache result
else Subsequent Call
OnceWrapper-->>Caller: Return cached result
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🔇 Additional comments (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
More templates
@orpc/client
@orpc/contract
@orpc/react-query
@orpc/openapi
@orpc/server
@orpc/shared
@orpc/standard-server
@orpc/standard-server-fetch
@orpc/standard-server-node
@orpc/vue-colada
@orpc/vue-query
@orpc/zod
commit: |
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/shared/src/iterator.ts (1)
1-7: Well-implemented async iterator type checkThe
isAsyncIteratorObjectfunction follows the ECMAScript specification for identifying async iterators. It correctly checks for both the presence of theSymbol.asyncIteratorproperty and verifies that it's a function.Consider whether the generic type parameters in
AsyncIteratorObject<any, any, any>could be more strictly typed for better type safety, depending on how this function is used throughout the codebase.packages/standard-server/src/types.ts (1)
11-11: Consider addressing thevoidin union type static analysis warning.The static analyzer flags the use of
voidin a union type as potentially confusing. While this is pre-existing code, consider replacingvoidwithundefinedfor clarity since the analyzer suggests this would be safer.- | AsyncIterator<unknown | void, unknown | void, undefined> + | AsyncIterator<unknown | undefined, unknown | undefined, undefined>🧰 Tools
🪛 Biome (1.9.4)
[error] 11-11: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 11-11: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/standard-server-node/src/event-source.ts (1)
13-14: Consider addressing static analysis warnings about void in union types.The static analyzer flags
voidin union types as potentially confusing. Consider usingundefinedinstead if semantically appropriate.-): AsyncGenerator<unknown | void, unknown | void, void> { +): AsyncGenerator<unknown | undefined, unknown | undefined, void> {And for the toEventStream parameter:
- iterator: AsyncIterator<unknown | void, unknown | void, void>, + iterator: AsyncIterator<unknown | undefined, unknown | undefined, void>,Also applies to: 84-85
🧰 Tools
🪛 Biome (1.9.4)
[error] 14-14: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 14-14: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/standard-server-fetch/src/event-source.ts (1)
13-13: Consider addressing static analysis warnings about void in union types.The static analyzer flags
voidin union types as potentially confusing. Consider usingundefinedinstead if semantically appropriate, similar to the suggestion for the Node implementation.-): AsyncGenerator<unknown | void, unknown | void, void> { +): AsyncGenerator<unknown | undefined, unknown | undefined, void> {And for the toEventStream parameter:
- iterator: AsyncIterator<unknown | void, unknown | void, void>, + iterator: AsyncIterator<unknown | undefined, unknown | undefined, void>,Also applies to: 83-84
🧰 Tools
🪛 Biome (1.9.4)
[error] 13-13: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 13-13: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (41)
packages/client/src/adapters/fetch/rpc-link.test.ts(1 hunks)packages/client/src/adapters/fetch/rpc-link.ts(1 hunks)packages/client/src/event-iterator.ts(3 hunks)packages/client/src/openapi/serializer.ts(2 hunks)packages/client/src/rpc/serializer.test.ts(1 hunks)packages/client/src/rpc/serializer.ts(3 hunks)packages/contract/src/event-iterator.ts(1 hunks)packages/server/src/adapters/standard/rpc-codec.ts(1 hunks)packages/shared/src/function.test.ts(1 hunks)packages/shared/src/function.ts(1 hunks)packages/shared/src/index.ts(1 hunks)packages/shared/src/iterator.test.ts(1 hunks)packages/shared/src/iterator.ts(1 hunks)packages/shared/src/json.test.ts(1 hunks)packages/shared/src/json.ts(1 hunks)packages/shared/src/object.test.ts(1 hunks)packages/shared/src/object.ts(1 hunks)packages/standard-server-fetch/package.json(1 hunks)packages/standard-server-fetch/playground/event-source.ts(1 hunks)packages/standard-server-fetch/src/body.test.ts(1 hunks)packages/standard-server-fetch/src/body.ts(2 hunks)packages/standard-server-fetch/src/event-source.test.ts(2 hunks)packages/standard-server-fetch/src/event-source.ts(4 hunks)packages/standard-server-fetch/src/request.test.ts(1 hunks)packages/standard-server-fetch/src/request.ts(1 hunks)packages/standard-server-fetch/tsconfig.json(1 hunks)packages/standard-server-node/package.json(1 hunks)packages/standard-server-node/playground/event-source.ts(1 hunks)packages/standard-server-node/src/body.test.ts(1 hunks)packages/standard-server-node/src/body.ts(2 hunks)packages/standard-server-node/src/event-source.test.ts(2 hunks)packages/standard-server-node/src/event-source.ts(4 hunks)packages/standard-server-node/src/request.ts(1 hunks)packages/standard-server/package.json(1 hunks)packages/standard-server/src/event-source/errors.ts(1 hunks)packages/standard-server/src/event-source/meta.test.ts(1 hunks)packages/standard-server/src/event-source/meta.ts(2 hunks)packages/standard-server/src/types.ts(1 hunks)packages/standard-server/src/utils.test.ts(0 hunks)packages/standard-server/src/utils.ts(0 hunks)packages/standard-server/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (2)
- packages/standard-server/src/utils.ts
- packages/standard-server/src/utils.test.ts
✅ Files skipped from review due to trivial changes (11)
- packages/standard-server-node/src/request.ts
- packages/standard-server-node/playground/event-source.ts
- packages/standard-server-fetch/src/request.ts
- packages/client/src/rpc/serializer.test.ts
- packages/standard-server-node/src/body.test.ts
- packages/client/src/adapters/fetch/rpc-link.test.ts
- packages/client/src/adapters/fetch/rpc-link.ts
- packages/standard-server-fetch/src/request.test.ts
- packages/server/src/adapters/standard/rpc-codec.ts
- packages/standard-server-fetch/src/body.test.ts
- packages/standard-server-fetch/playground/event-source.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/standard-server/src/types.ts
[error] 11-11: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 11-11: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/standard-server-node/src/event-source.ts
[error] 14-14: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 14-14: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 84-84: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 84-84: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/standard-server-fetch/src/event-source.ts
[error] 13-13: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 13-13: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 83-83: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 83-83: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (43)
packages/contract/src/event-iterator.ts (1)
4-4: Import source updated correctly.The import for
isAsyncIteratorObjecthas been updated from@orpc/standard-serverto@orpc/sharedas part of the refactoring effort to centralize shared utilities.packages/standard-server/tsconfig.json (1)
6-8: TypeScript project reference correctly added.Adding a reference to the shared package in the tsconfig.json is appropriate since functionality is being moved there, maintaining proper project dependencies.
packages/standard-server-fetch/tsconfig.json (1)
7-8: References array correctly updated.The references array now properly includes both
../standard-serverand../shared, reflecting the package's dependencies after the refactoring.packages/standard-server-fetch/package.json (1)
44-44: Added dependency on @orpc/sharedThis addition aligns with the PR objective to update the standard server, where functionality is being moved from
@orpc/standard-serverto@orpc/shared.packages/shared/src/index.ts (1)
5-6: New exports from shared modulesThe additional exports make functionality from the
iteratorandjsonmodules available through the main entry point, which is a good practice for module organization. This change supports the PR's goal of moving functionality to the shared package.packages/shared/src/json.test.ts (1)
1-7: Good test coverage for parseEmptyableJSONThe test cases are comprehensive, covering:
- Empty string handling (returns undefined)
- Empty object parsing
- Valid JSON string parsing
This ensures the function behaves correctly for all expected input types.
packages/shared/src/function.test.ts (1)
1-14: Well-structured test for the 'once' memoization functionThe test properly verifies that:
- The wrapped function returns the same result on multiple calls (using
toBefor reference equality)- The underlying function is only called once
This confirms that the
oncefunction correctly caches results and prevents redundant executions.packages/standard-server-node/package.json (1)
44-44: Adding dependency on shared packageAdding
@orpc/sharedas a dependency aligns with the PR objective of refactoring imports by moving utilities from@orpc/standard-serverto@orpc/shared, which centralizes shared code and improves maintainability.packages/standard-server/package.json (1)
44-45: Dependency changes align with code refactoringThe addition of
@orpc/sharedand removal oftype-festaligns with the refactoring effort mentioned in the PR summary, where utilities are being centralized in the shared package andJsonValuetypes are being replaced withunknown.packages/shared/src/object.ts (1)
92-97: Well-implemented TypeScript type guardThe new
isTypescriptObjectfunction is a good addition that correctly implements a type guard for TypeScript'sobjecttype (which includes both objects and functions). The function correctly handles null values and uses appropriate type narrowing.packages/shared/src/object.test.ts (1)
3-13: Excellent test coverage for isTypescriptObject function.The test cases comprehensively cover both positive and negative scenarios, verifying the function correctly identifies TypeScript object types versus primitive values.
packages/shared/src/iterator.test.ts (1)
3-16: Well-structured test coverage for isAsyncIteratorObject function.The test suite thoroughly verifies the function's behavior with both positive and negative cases, including correct handling of async vs sync generators and malformed iterator objects.
packages/client/src/event-iterator.ts (3)
2-2: Import statement correctly updated to use shared package.The import of
isTypescriptObjectfrom@orpc/sharedaligns with the PR objective of shifting imports from@orpc/standard-serverto@orpc/shared.
22-22: Type check updated to use isTypescriptObject.The condition has been properly updated to use the new shared type-checking utility.
39-39: Type check updated to use isTypescriptObject.The condition has been properly updated to use the new shared type-checking utility, maintaining consistency with the earlier change.
packages/shared/src/function.ts (1)
3-16: Clean implementation of the once utility function.The function correctly implements a memoization pattern to ensure a function is executed only once, with subsequent calls returning the cached result.
For future consideration:
- This implementation only works for parameterless functions
- It doesn't handle exceptions in the original function
- There's no mechanism to reset the cache
packages/standard-server-node/src/body.ts (2)
5-6: Module imports reorganization looks goodThe reallocation of utility functions (
isAsyncIteratorObjectandparseEmptyableJSON) to@orpc/sharedis a good practice for centralizing common utilities.
44-44:❓ Verification inconclusive
Verify content type handling change
This change narrows content type handling from all
text/*types to onlytext/plain, which could impact how other text formats (HTML, CSS, etc.) are processed.
🏁 Script executed:
#!/bin/bash # Check for any other code that might be relying on handling other text/* content types rg "text/(?!plain|event-stream)" --type ts --glob !packages/standard-server-node/src/body.tsLength of output: 332
Below is a new verification command using the PCRE2 flag. It searches for references to other
text/*content types (excludingtext/plainandtext/event-stream) in the repository. Please run it and manually verify that no critical parts of the codebase rely on other text formats (e.g., HTML, CSS, etc.).Once you have the output, please review it to ensure that narrowing the content type handling to
text/plainis safe.
🏁 Script executed:
[run_scripts] #!/bin/bash rg "text/(?!(plain|event-stream))" --pcre2 --type ts --glob '!packages/standard-server-node/src/body.ts'Length of output: 623
Action: Confirm Intended Content Type Processing Behavior
The change in
packages/standard-server-node/src/body.tsnow exclusively processes content types starting withtext/plain. However, our repository search identified several instances in the playground examples wheretext/htmlis explicitly used:
- playgrounds/contract-openapi/src/main.ts: Sets
Content-Typetotext/html- playgrounds/expressjs/src/main.ts: Sets
Content-Typetotext/html- playgrounds/nuxt/server/routes/scalar.ts: Sets
Content-Typetotext/html- playgrounds/openapi/src/main.ts: Sets
Content-Typetotext/html- playgrounds/nextjs/src/app/scalar/route.ts: Sets
Content-Typetotext/htmlPlease verify that narrowing the check to only
text/plainin the standard server logic is intentional. Confirm whether content types liketext/htmlare expected to be processed differently (or elsewhere) without causing regressions for clients relying on broadertext/*handling.packages/client/src/rpc/serializer.ts (2)
1-2: Module imports reorganization looks goodThe relocation of
isAsyncIteratorObjectto@orpc/sharedaligns with the broader refactoring efforts seen across the project.
19-20: Type assertions removal improves type safetyRemoving the explicit type casting to
JsonValueis a good change that aligns with usingunknowninErrorEvent. This makes the code more type-safe by not assuming the data structure prematurely.Also applies to: 25-26, 81-82
packages/standard-server/src/event-source/errors.ts (1)
6-6: Improved type flexibility withunknownChanging from
undefined | JsonValuetounknownimproves flexibility while maintaining type safety. Theunknowntype requires explicit type checking before use, which helps prevent type-related errors.Also applies to: 10-10
packages/standard-server/src/event-source/meta.ts (1)
2-2: Good standardization of type checkingUsing the shared
isTypescriptObjectutility instead of a local implementation promotes consistency across the codebase. This is a good practice for maintainability and reduces the chance of subtle type-checking inconsistencies.Also applies to: 30-31
packages/standard-server/src/types.ts (1)
7-7: Type migration fromJsonValuetounknown.The change from
JsonValuetounknownmakes the type more permissive, which increases flexibility but requires more explicit type checking at consumption sites. This aligns with the overall refactoring effort in the PR to shift imports and types to@orpc/shared.Also applies to: 11-11
packages/standard-server-fetch/src/body.ts (2)
2-3: Reorganized imports to use shared package.The reorganization of imports aligns well with the overall refactoring effort, moving utility functions to
@orpc/shared. This promotes better code sharing and follows the PR's objectives.
44-44: Narrowed text content type handling to only plain text.The condition has been changed from handling all text types (
text/) to only handling plain text (text/plain). This is a significant behavioral change that could affect how other text subtypes are processed.Does your application expect to handle other text subtypes (like
text/html,text/csv, etc.) that would now be processed differently? With this change, such content types will fall through to the blob handler on lines 48-51 instead.packages/standard-server-fetch/src/event-source.test.ts (2)
1-2: MovedisAsyncIteratorObjectimport to shared package.Moving the utility function to the shared package is consistent with the broader refactoring effort and improves modularity.
186-186: Explicitly passingundefinedtogenerator.return().Making the parameter explicit improves code clarity by showing the intent, although functionally it's equivalent to the previous implementation where the parameter was implicitly undefined.
packages/standard-server-node/src/event-source.test.ts (2)
2-3: MovedisAsyncIteratorObjectimport to shared package.Moving the utility function to the shared package is consistent with the broader refactoring effort and improves modularity.
187-187: Explicitly passingundefinedtogenerator.return().Making the parameter explicit improves code clarity by showing the intent, although functionally it's equivalent to the previous implementation where the parameter was implicitly undefined.
packages/standard-server/src/event-source/meta.test.ts (2)
1-1: Imports aligned with implementation changes.The removal of
isEventMetaContainerfrom imports reflects the architectural change where this function has been replaced byisTypescriptObjectfrom@orpc/shared. Good job keeping imports minimal.
3-16: Well-structured tests for get/withEventMeta.The tests are comprehensive, checking both positive cases (applying and retrieving metadata) and error scenarios (invalid inputs). Error checks verify proper validation for newline characters in ID and invalid retry values.
packages/client/src/openapi/serializer.ts (3)
1-2: Import realignment is consistent with the architecture changes.Moving the import of
isAsyncIteratorObjectfrom@orpc/standard-serverto@orpc/sharedmatches the broader refactoring pattern described in the PR.
22-23: Proper removal of JsonValue type assertion.Removing the type assertion improves type safety by letting the TypeScript compiler infer the appropriate type.
28-29: Type assertion cleanup is consistent.Similar to the previous change, this improves type safety by removing unnecessary type assertions.
packages/standard-server-node/src/event-source.ts (5)
2-2: Import alignment with architectural changes.Adding
isTypescriptObjectfrom@orpc/sharedand movingparseEmptyableJSONimport keeps code in line with the refactoring pattern seen across the PR.
34-36: Implementation aligned with type system changes.Replacing
isEventMetaContainerwithisTypescriptObjectaligns with the architectural changes while maintaining the same logical flow.
55-57: Consistent implementation of type checking.The implementation maintains consistency with the earlier change, using
isTypescriptObjectfor type checking.
84-85: Type signature broadened consistently.The parameter type change from
JsonValue | voidtounknown | voidmaintains consistency with the return type change intoEventIterator.🧰 Tools
🪛 Biome (1.9.4)
[error] 84-84: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 84-84: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
13-14:✅ Verification successful
Type signature broadened from JsonValue to unknown.
Changing the return type from
JsonValue | voidtounknown | voidmakes the function more flexible but could potentially allow more runtime type errors if consumers aren't careful.
🏁 Script executed:
#!/bin/bash # Check for usages of toEventIterator to identify potential issues with type changes rg -A 3 "toEventIterator\(" --type tsLength of output: 5226
Revisiting the Type Signature Change in
toEventIteratorAfter verifying the usages of
toEventIteratoracross both thestandard-server-nodeandstandard-server-fetchpackages (including their respective tests), the change in return type—from a more restrictiveJsonValue | voidto a broaderunknown | void—appears to be applied consistently. While this broadening increases flexibility, it also shifts the responsibility for type validation onto the consumer. There are no immediate runtime type issues evident from the usage patterns, but please ensure that any consumers performing operations on yielded values include necessary type checks.
Affected Location:
packages/standard-server-node/src/event-source.ts(Lines 13-14)Observations:
- Both node and fetch implementations now return
AsyncGenerator<unknown | void, unknown | void, void>.- Test suites (e.g., in
event-source.test.ts) validate that the returned generators conform to the required async iterator interface, without relying on the previously specific type.Overall, the change looks intentional and consistent. However, please remain mindful of any runtime type assumptions in consumer code.
🧰 Tools
🪛 Biome (1.9.4)
[error] 14-14: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 14-14: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/standard-server-fetch/src/event-source.ts (4)
1-1: Import alignment with architectural changes.Importing both
isTypescriptObjectandparseEmptyableJSONfrom@orpc/sharedaligns with the refactoring to consolidate utility functions in the shared package.
13-13: Type signature broadened from JsonValue to unknown.Similar to the changes in packages/standard-server-node/src/event-source.ts, broadening the type makes the function more flexible but requires careful type checking by consumers.
🧰 Tools
🪛 Biome (1.9.4)
[error] 13-13: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 13-13: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
33-35: Implementation aligned with type system changes.Both instances of replacing
isEventMetaContainerwithisTypescriptObjectmaintain the same logical flow while aligning with the architectural changes.Also applies to: 54-56
83-84: Type signature broadened consistently.The parameter type change maintains consistency with the return type change in
toEventIteratorand the parallel changes in the Node implementation.🧰 Tools
🪛 Biome (1.9.4)
[error] 83-83: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 83-83: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
| export function parseEmptyableJSON(text: string): unknown { | ||
| if (!text) { | ||
| return undefined | ||
| } | ||
|
|
||
| return JSON.parse(text) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for JSON parsing.
The current implementation of parseEmptyableJSON doesn't handle JSON parsing errors, which could lead to uncaught exceptions when parsing invalid JSON strings.
Consider adding error handling:
export function parseEmptyableJSON(text: string): unknown {
if (!text) {
return undefined
}
- return JSON.parse(text)
+ try {
+ return JSON.parse(text)
+ } catch (error) {
+ console.error('Failed to parse JSON:', error)
+ return undefined
+ }
}Additionally, consider documenting the function's behavior when receiving empty strings or invalid JSON.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function parseEmptyableJSON(text: string): unknown { | |
| if (!text) { | |
| return undefined | |
| } | |
| return JSON.parse(text) | |
| } | |
| export function parseEmptyableJSON(text: string): unknown { | |
| if (!text) { | |
| return undefined | |
| } | |
| try { | |
| return JSON.parse(text) | |
| } catch (error) { | |
| console.error('Failed to parse JSON:', error) | |
| return undefined | |
| } | |
| } |
Summary by CodeRabbit
New Features
oncefor ensuring single execution of a function.parseEmptyableJSONfor safely parsing JSON strings.Refactor
isTypescriptObject.Tests
Chores