Conversation
🚀 Snapshot Release (
|
| Package | Version | Info |
|---|---|---|
@graphql-tools/executor-graphql-ws |
2.0.0-alpha-8eea33065b3fff7c92ce74a6217f07f40fd2987f |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/gateway |
1.9.2-alpha-8eea33065b3fff7c92ce74a6217f07f40fd2987f |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-opentelemetry |
1.3.39-alpha-8eea33065b3fff7c92ce74a6217f07f40fd2987f |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-prometheus |
1.3.27-alpha-8eea33065b3fff7c92ce74a6217f07f40fd2987f |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/gateway-runtime |
1.4.11-alpha-8eea33065b3fff7c92ce74a6217f07f40fd2987f |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-ws |
1.0.0-alpha-8eea33065b3fff7c92ce74a6217f07f40fd2987f |
npm ↗︎ unpkg ↗︎ |
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Bun Docker Image)The latest changes of this PR are available as image on GitHub Container Registry (based on the declared |
🚀 Snapshot Release (Node Docker Image)The latest changes of this PR are available as image on GitHub Container Registry (based on the declared |
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Binary for
|
95b7dac to
6e06e7b
Compare
There was a problem hiding this comment.
lgtm. @enisdenjo anything to update in docs or examples?
Also please check my comment about the changesets, and let's wait for @ardatan 's review as well.
🚀 Snapshot Release (Binary for
|
bd0af4a to
32c59ce
Compare
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis pull request updates the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DisposableServer
participant getAvailablePort
participant WS_Server
Client->>DisposableServer: createDisposableWebSocketServer()
DisposableServer->>getAvailablePort: Request available port
getAvailablePort-->>DisposableServer: Return port number
DisposableServer->>WS_Server: Initialize WebSocket server with port
WS_Server-->>DisposableServer: Handle connection events
DisposableServer->>Client: Return server instance with disposal method
Assessment against linked issues
Possibly related issues
✨ Finishing Touches
🪧 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
.changeset/silent-dingos-warn.md (1)
1-6: Version bump should be major for breaking changes.The PR upgrades graphql-ws from v5 to v6 which includes breaking changes. According to semver, this should be a major version bump rather than minor.
Apply this diff to update the version bump:
-'@graphql-tools/executor-graphql-ws': minor +'@graphql-tools/executor-graphql-ws': major
🧹 Nitpick comments (6)
.changeset/swift-scissors-end.md (1)
5-8: Clarify the description regarding executor options removal.
The current wording "Executor options don't exist graphql-ws dependency options" is unclear. Consider rephrasing to better convey that the dependency on graphql‑ws types for executor options has been removed, and that while some options remain exposed, users should supply their own client instance for further customization.Suggested diff:
-Executor options don't exist graphql-ws dependency options -Removing the dependency on the types. Some options are still exposed, but if you want to further customise the graphql-ws client, you should pass your own instance of the client instead. +The dependency on graphql‑ws types for executor options has been removed. +Some options remain exposed; however, for further customization of the graphql‑ws client, please pass your own client instance.packages/transports/ws/src/index.ts (1)
77-123: WebSocket client setup and event logging are well-structured.
The lazy initialization, connection callbacks, and final disposal logic align nicely with best practices. Consider whether any retry or exponential backoff strategy is needed for error handling.packages/transports/ws/tests/ws.spec.ts (1)
18-18: Define explicit type shape instead of empty object.Using
{}as a type is discouraged as it means "any non-nullable value". Consider defining an explicit interface or type alias for the server context.-type TServerOptions = ServerOptions<{}, WSExtra | BunExtra>; +interface ServerContext {} +type TServerOptions = ServerOptions<ServerContext, WSExtra | BunExtra>;🧰 Tools
🪛 Biome (1.9.4)
[error] 18-18: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
.changeset/six-planes-change.md (2)
1-3: Changeset Version Bump Declaration.The changeset correctly marks
@graphql-mesh/transport-wsas a major change and@graphql-hive/gatewayas a patch. Ensure that the release notes and migration guides detail any breaking changes or necessary migration steps.
6-9: Documentation Clarity in Changeset Description.A static analysis hint suggests a possible missing comma. To improve readability, consider revising the sentence to:
“WebSocket transport options allow configuring only
connectionParams, and in most cases you won't need to configure the underlying graphql‑ws client any further.”🧰 Tools
🪛 LanguageTool
[uncategorized] ~8-~8: Possible missing comma found.
Context: ...onlyconnectionParamsIn most of the cases you won't need to configure the underly...(AI_HYDRA_LEO_MISSING_COMMA)
internal/testing/package.json (1)
13-19: Consider pinning dependency versions.Using caret ranges (
^) for dependencies could lead to unexpected breaking changes. Since this is an internal testing module, it's important to have reproducible test environments.Apply this diff to pin the versions:
- "@graphql-tools/utils": "^10.7.2", + "@graphql-tools/utils": "10.7.2", "@types/node": "^22.7.5", "@whatwg-node/disposablestack": "^0.0.5", "@whatwg-node/fetch": "^0.10.1", - "graphql": "^16.10.0", + "graphql": "16.10.0", "jest-leak-detector": "29.7.0", - "ws": "^8.18.0" + "ws": "8.18.0"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
examples/federation-subscriptions-passthrough/example.tar.gzis excluded by!**/*.gzexamples/federation-subscriptions-passthrough/package-lock.jsonis excluded by!**/package-lock.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (29)
.changeset/@graphql-hive_gateway-481-dependencies.md(1 hunks).changeset/@graphql-hive_gateway-runtime-481-dependencies.md(1 hunks).changeset/@graphql-mesh_transport-ws-481-dependencies.md(1 hunks).changeset/@graphql-tools_executor-graphql-ws-481-dependencies.md(1 hunks).changeset/silent-dingos-warn.md(1 hunks).changeset/six-planes-change.md(1 hunks).changeset/swift-scissors-end.md(1 hunks).changeset/tame-garlics-itch.md(1 hunks).changeset/violet-scissors-judge.md(1 hunks).changeset/warm-tigers-lie.md(1 hunks)e2e/federation-subscriptions-passthrough/package.json(1 hunks)e2e/federation-subscriptions-passthrough/services/products/server.ts(1 hunks)examples/federation-subscriptions-passthrough/package.json(1 hunks)examples/federation-subscriptions-passthrough/services/products/server.ts(1 hunks)internal/proc/src/index.ts(0 hunks)internal/testing/package.json(1 hunks)internal/testing/src/index.ts(1 hunks)internal/testing/src/server.ts(2 hunks)packages/executors/graphql-ws/package.json(1 hunks)packages/executors/graphql-ws/src/index.ts(2 hunks)packages/executors/graphql-ws/tests/graphql-ws.test.ts(1 hunks)packages/gateway/package.json(1 hunks)packages/gateway/src/servers/bun.ts(2 hunks)packages/gateway/src/servers/nodeHttp.ts(2 hunks)packages/runtime/package.json(1 hunks)packages/runtime/src/getGraphQLWSOptions.ts(2 hunks)packages/transports/ws/package.json(1 hunks)packages/transports/ws/src/index.ts(3 hunks)packages/transports/ws/tests/ws.spec.ts(2 hunks)
💤 Files with no reviewable changes (1)
- internal/proc/src/index.ts
✅ Files skipped from review due to trivial changes (10)
- .changeset/@graphql-hive_gateway-runtime-481-dependencies.md
- .changeset/tame-garlics-itch.md
- packages/executors/graphql-ws/tests/graphql-ws.test.ts
- .changeset/@graphql-hive_gateway-481-dependencies.md
- .changeset/@graphql-tools_executor-graphql-ws-481-dependencies.md
- .changeset/warm-tigers-lie.md
- internal/testing/src/index.ts
- .changeset/@graphql-mesh_transport-ws-481-dependencies.md
- examples/federation-subscriptions-passthrough/services/products/server.ts
- e2e/federation-subscriptions-passthrough/services/products/server.ts
🧰 Additional context used
📓 Path-based instructions (14)
e2e/federation-subscriptions-passthrough/package.json (2)
Pattern e2e/**: This directory includes end-to-end tests for the gateway.
The examples directory is generated based on the code in this directory.
Pattern **: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.
In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding console project.
Use the global knowledge feature to search for PRs in graphql-hive/console repository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/transports/ws/package.json (3)
Pattern packages/**: In this directory we keep all packages relevant to the gateway.
In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using yarn changeset with a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mention minor semver change. When a bug is fixed, patch needs to be used. The major bump is used for a PR that has breaking changes.
Pattern packages/transports/**: The transports configures how to deal with HTTP requests that are made from the GraphQL gateway to the GraphQL subgraphs. Transports manipulates the HTTP calls, headers, protocols and others.
Pattern **: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.
In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding console project.
Use the global knowledge feature to search for PRs in graphql-hive/console repository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/gateway/package.json (3)
Pattern packages/**: In this directory we keep all packages relevant to the gateway.
In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using yarn changeset with a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mention minor semver change. When a bug is fixed, patch needs to be used. The major bump is used for a PR that has breaking changes.
Pattern packages/gateway/**: The main runtime for the Hive gateway.
This package is CLI that runs the gateway and configures the internals of the gateway function.
Pattern **: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.
In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding console project.
Use the global knowledge feature to search for PRs in graphql-hive/console repository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
examples/federation-subscriptions-passthrough/package.json (2)
Pattern examples/**: In this directory we keep all the examples. Examples are generated using a scheduled CI pipeline.
When changes are made to Gateway config, we expect to see some changes in the examples directory.
Pattern **: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.
In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding console project.
Use the global knowledge feature to search for PRs in graphql-hive/console repository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/runtime/package.json (2)
Pattern packages/**: In this directory we keep all packages relevant to the gateway.
In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using yarn changeset with a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mention minor semver change. When a bug is fixed, patch needs to be used. The major bump is used for a PR that has breaking changes.
Pattern **: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.
In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding console project.
Use the global knowledge feature to search for PRs in graphql-hive/console repository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
internal/testing/src/server.ts (1)
Pattern **: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.
In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding console project.
Use the global knowledge feature to search for PRs in graphql-hive/console repository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/executors/graphql-ws/package.json (2)
Pattern packages/**: In this directory we keep all packages relevant to the gateway.
In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using yarn changeset with a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mention minor semver change. When a bug is fixed, patch needs to be used. The major bump is used for a PR that has breaking changes.
Pattern **: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.
In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding console project.
Use the global knowledge feature to search for PRs in graphql-hive/console repository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
internal/testing/package.json (1)
Pattern **: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.
In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding console project.
Use the global knowledge feature to search for PRs in graphql-hive/console repository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/gateway/src/servers/nodeHttp.ts (3)
Pattern packages/**: In this directory we keep all packages relevant to the gateway.
In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using yarn changeset with a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mention minor semver change. When a bug is fixed, patch needs to be used. The major bump is used for a PR that has breaking changes.
Pattern packages/gateway/**: The main runtime for the Hive gateway.
This package is CLI that runs the gateway and configures the internals of the gateway function.
Pattern **: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.
In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding console project.
Use the global knowledge feature to search for PRs in graphql-hive/console repository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/gateway/src/servers/bun.ts (3)
Pattern packages/**: In this directory we keep all packages relevant to the gateway.
In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using yarn changeset with a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mention minor semver change. When a bug is fixed, patch needs to be used. The major bump is used for a PR that has breaking changes.
Pattern packages/gateway/**: The main runtime for the Hive gateway.
This package is CLI that runs the gateway and configures the internals of the gateway function.
Pattern **: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.
In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding console project.
Use the global knowledge feature to search for PRs in graphql-hive/console repository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/transports/ws/tests/ws.spec.ts (3)
Pattern packages/**: In this directory we keep all packages relevant to the gateway.
In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using yarn changeset with a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mention minor semver change. When a bug is fixed, patch needs to be used. The major bump is used for a PR that has breaking changes.
Pattern packages/transports/**: The transports configures how to deal with HTTP requests that are made from the GraphQL gateway to the GraphQL subgraphs. Transports manipulates the HTTP calls, headers, protocols and others.
Pattern **: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.
In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding console project.
Use the global knowledge feature to search for PRs in graphql-hive/console repository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/runtime/src/getGraphQLWSOptions.ts (2)
Pattern packages/**: In this directory we keep all packages relevant to the gateway.
In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using yarn changeset with a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mention minor semver change. When a bug is fixed, patch needs to be used. The major bump is used for a PR that has breaking changes.
Pattern **: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.
In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding console project.
Use the global knowledge feature to search for PRs in graphql-hive/console repository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/executors/graphql-ws/src/index.ts (2)
Pattern packages/**: In this directory we keep all packages relevant to the gateway.
In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using yarn changeset with a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mention minor semver change. When a bug is fixed, patch needs to be used. The major bump is used for a PR that has breaking changes.
Pattern **: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.
In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding console project.
Use the global knowledge feature to search for PRs in graphql-hive/console repository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/transports/ws/src/index.ts (3)
Pattern packages/**: In this directory we keep all packages relevant to the gateway.
In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using yarn changeset with a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mention minor semver change. When a bug is fixed, patch needs to be used. The major bump is used for a PR that has breaking changes.
Pattern packages/transports/**: The transports configures how to deal with HTTP requests that are made from the GraphQL gateway to the GraphQL subgraphs. Transports manipulates the HTTP calls, headers, protocols and others.
Pattern **: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.
In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding console project.
Use the global knowledge feature to search for PRs in graphql-hive/console repository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
🪛 Biome (1.9.4)
packages/transports/ws/tests/ws.spec.ts
[error] 18-18: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🪛 LanguageTool
.changeset/six-planes-change.md
[uncategorized] ~8-~8: Possible missing comma found.
Context: ...only connectionParams In most of the cases you won't need to configure the underly...
(AI_HYDRA_LEO_MISSING_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (25)
- GitHub Check: Binary built on ubuntu-24.04-arm
- GitHub Check: Binary built on windows-latest
- GitHub Check: Binary built on macos-14
- GitHub Check: Binary built on macos-13
- GitHub Check: Binary built on ubuntu-latest
- GitHub Check: Bun Docker image
- GitHub Check: Node Docker image
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: Benchmark / node / 10 items
- GitHub Check: Leaks / Node v22
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Leaks / Node v20
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Leaks / Node v18
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Bundle
🔇 Additional comments (23)
e2e/federation-subscriptions-passthrough/package.json (1)
18-18: Dependency Version Update forgraphql-wsThe update from
"graphql-ws": "^5.16.0"to"graphql-ws": "^6.0.2"correctly aligns with the PR’s objective to upgrade to version 6. Please ensure that any client-specific changes required by the new API (such as adjustments to custom client options) are appropriately reflected in the test suite and implementation. Additionally, verify that this update does not break compatibility with the existing WebSocket transport tests.examples/federation-subscriptions-passthrough/package.json (1)
16-16: Dependency Version Upgrade: Verify Version ConsistencyThe upgrade to
"graphql-ws": "^6.0.2"aligns with the PR’s objective to move to v6 of the package. However, note that similar updates in other parts of the project reference version^6.0.3. Please verify if the patch-level discrepancy is intentional or if aligning to the same patch version (i.e.^6.0.3) would be more consistent across the ecosystem..changeset/swift-scissors-end.md (2)
1-3: Major version bump annotation is clear.
The changeset header correctly indicates a major version update for the@graphql-tools/executor-graphql-wspackage, aligning with the breaking changes introduced by upgrading tographql-wsv6.
9-21: Example code snippet is clear and useful.
The provided TypeScript example effectively demonstrates how to create a GraphQL WebSocket executor using a custom client viabuildGraphQLWSExecutorandcreateClient. This serves as a practical guide for users to update their implementation..changeset/violet-scissors-judge.md (2)
1-4: Review YAML Front Matter for Version Bumps
The YAML front matter is correctly formatted and specifies the version bumps for the packages. However, the PR objective mentions the release of v1 for the WebSocket transport, which might imply a major version bump. Please verify that setting'@graphql-mesh/transport-ws': minoraligns with your versioning strategy (especially if the previous version was pre-1.0, where a minor bump might effectively release v1).
6-8: Clear Migration Instructions for Custom Configurations
The release note clearly instructs users to upgrade to graphql-ws v6 and provides a useful link to the changelog for detailed migration guidance. For further clarity, consider expanding on the specific breaking changes or potential configuration adjustments that custom setups might require.packages/transports/ws/src/index.ts (3)
13-14: Imports align well with the graphql-ws v6 upgrade.
These imports correctly reference the updated types to support the new WebSocket client creation.
27-34: Interface addition looks good.
DefiningconnectionParamsin theWSTransportOptionsinterface is clear and flexible, allowing important metadata to be passed during connection initialization.
39-41: Function signature refactor appears valid.
Accepting aClientand returning aDisposableAsyncExecutorcentralizes the logic for client creation and cleanup.packages/runtime/src/getGraphQLWSOptions.ts (3)
5-10: Newly imported types support backward compatibility.
Bringing inSubscribeMessageandSubscribePayloadis consistent with handling different versions ofgraphql-ws.
32-40: Smart fallback ensures support for both v5 and v6 messages.
This logic neatly differentiates between string-based (v6) and object-based (v5) message formats, preserving backward compatibility.
49-51: Parsing and assigning subscription payload fields is correct.
The references topayload.operationName,payload.query, andpayload.variablescorrectly populate theExecutionArgs.packages/gateway/src/servers/bun.ts (2)
3-3: Updated import path reflects graphql-ws v6 structure.
Using'graphql-ws/use/bun'is the correct approach for the new version of the library.
43-43: Dynamically importing the new handler is appropriate.
Switching to themakeHandlerfrom'graphql-ws/use/bun'ensures compatibility with the upgraded library.packages/gateway/src/servers/nodeHttp.ts (1)
7-7: LGTM!The import path updates align with the graphql-ws v6 module structure.
Also applies to: 88-88
packages/executors/graphql-ws/src/index.ts (2)
16-48: LGTM! Well-documented interface with improved type safety.The GraphQLWSExecutorOptions interface is now more explicit and better documented, improving type safety and developer experience.
76-87: LGTM! Improved client configuration.The client configuration is now more explicit and type-safe, properly handling headers and connection parameters.
packages/transports/ws/tests/ws.spec.ts (2)
20-103: LGTM! Well-structured test helper.The createTServer function is well-organized and handles both Bun and Node.js environments appropriately. The cleanup logic is properly implemented using the disposable pattern.
154-158: Consider investigating test leaks.Multiple test assertions are skipped due to leaks. This might indicate underlying memory management issues that should be investigated.
Also applies to: 172-176, 214-218
✅ Verification successful
Memory Leak Test Patterns Confirmed in ws.spec.ts and Across Test Suites
The investigation shows that the memory leak patterns are already known and intentionally handled via the
LEAK_TESTenvironment variable. Similar patterns in other test files and the dedicated leak-detector test confirm that the test skips and disposal mechanisms are by design rather than indicating unexpected memory management issues.
- The leak-related assertions are conditionally run/skipped using
process.env['LEAK_TEST'].- Disposal functions (e.g.,
dispose()) are consistently applied to clean up resources.- A dedicated leak detection test exists (
internal/testing/tests/leak-detector.test.ts).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for memory leak patterns in the codebase # Look for similar patterns of skipped tests due to leaks rg -A 5 "LEAK_TEST|this assertion leaks" # Look for dispose/cleanup patterns ast-grep --pattern 'dispose($$$)'Length of output: 5969
packages/executors/graphql-ws/package.json (1)
45-45: Upgrade to graphql‑ws v6.0.3 Dependency.The dependency update from an earlier 5.x version to ^6.0.3 meets the upgrade objective. Please verify that all internal API usages and exported interfaces (such as in GraphQLWSExecutorOptions) are compatible with any breaking changes introduced in graphql‑ws v6.
packages/transports/ws/package.json (1)
52-53: Update of WebSocket Transport Dependencies.The upgrade to
"graphql-ws": "^6.0.3"along with the addition of"isomorphic-ws": "^5.0.0"aligns with the broader dependency bump across packages. Ensure that the transport’s implementation fully leverages the new client creation options provided by graphql‑ws v6 and remains consistent with related packages.packages/runtime/package.json (1)
81-81: Upgrade in Gateway‑Runtime Dependency.The update to
"graphql-ws": "^6.0.3"is consistent with the coordinated upgrade. Please confirm that any runtime logic interfacing with graphql‑ws handles the changes in behaviors or configuration introduced in the new version.packages/gateway/package.json (1)
86-86: graphql‑ws Version Bump in Gateway.The dependency version change to
"graphql-ws": "^6.0.3"is in line with the overall upgrade across the ecosystem. Verify that any client configuration or usage within the gateway aligns with the new API of graphql‑ws v6.
| export function getAvailablePort(): Promise<number> { | ||
| const deferred = createDeferred<number>(); | ||
| const server = createServer(); | ||
| server.once('error', (err) => deferred.reject(err)); | ||
| server.listen(0, () => { | ||
| try { | ||
| const addressInfo = server.address() as AddressInfo; | ||
| server.close((err) => { | ||
| if (err) { | ||
| return deferred.reject(err); | ||
| } | ||
|
|
||
| return deferred.resolve(addressInfo.port); | ||
| }); | ||
| } catch (err) { | ||
| return deferred.reject(err); | ||
| } | ||
| }); | ||
| return deferred.promise; | ||
| } |
There was a problem hiding this comment.
Fix potential memory leak in server error handling.
The server is not properly cleaned up if an error occurs before the listen event. This could lead to a memory leak.
Apply this diff to ensure proper cleanup:
export function getAvailablePort(): Promise<number> {
const deferred = createDeferred<number>();
const server = createServer();
- server.once('error', (err) => deferred.reject(err));
+ server.once('error', (err) => {
+ server.close();
+ deferred.reject(err);
+ });
server.listen(0, () => {
try {
const addressInfo = server.address() as AddressInfo;
server.close((err) => {
if (err) {
return deferred.reject(err);
}
return deferred.resolve(addressInfo.port);
});
} catch (err) {
+ server.close();
return deferred.reject(err);
}
});
return deferred.promise;
}📝 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 getAvailablePort(): Promise<number> { | |
| const deferred = createDeferred<number>(); | |
| const server = createServer(); | |
| server.once('error', (err) => deferred.reject(err)); | |
| server.listen(0, () => { | |
| try { | |
| const addressInfo = server.address() as AddressInfo; | |
| server.close((err) => { | |
| if (err) { | |
| return deferred.reject(err); | |
| } | |
| return deferred.resolve(addressInfo.port); | |
| }); | |
| } catch (err) { | |
| return deferred.reject(err); | |
| } | |
| }); | |
| return deferred.promise; | |
| } | |
| export function getAvailablePort(): Promise<number> { | |
| const deferred = createDeferred<number>(); | |
| const server = createServer(); | |
| server.once('error', (err) => { | |
| server.close(); | |
| deferred.reject(err); | |
| }); | |
| server.listen(0, () => { | |
| try { | |
| const addressInfo = server.address() as AddressInfo; | |
| server.close((err) => { | |
| if (err) { | |
| return deferred.reject(err); | |
| } | |
| return deferred.resolve(addressInfo.port); | |
| }); | |
| } catch (err) { | |
| server.close(); | |
| return deferred.reject(err); | |
| } | |
| }); | |
| return deferred.promise; | |
| } |
| export async function createDisposableWebSocketServer() { | ||
| const port = await getAvailablePort(); | ||
| const server = new WebSocketServer({ port }); | ||
|
|
||
| const sockets = new Set<WebSocket>(); | ||
| server.on('connection', (conn) => { | ||
| sockets.add(conn); | ||
| conn.once('close', () => sockets.delete(conn)); | ||
| }); | ||
|
|
||
| const url = `ws://localhost:${port}`; | ||
|
|
||
| return { | ||
| url, | ||
| server, | ||
| [DisposableSymbols.asyncDispose]() { | ||
| for (const socket of sockets) { | ||
| socket.close(1001, 'Going Away'); | ||
| } | ||
| return new Promise<void>((resolve, reject) => { | ||
| server.close((err) => { | ||
| if (err) { | ||
| reject(err); | ||
| } else { | ||
| resolve(); | ||
| } | ||
| }); | ||
| }); | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix potential memory leak in WebSocket server cleanup.
The WebSocket server cleanup could be improved to handle errors during connection closure and ensure proper cleanup of resources.
Apply this diff to ensure proper cleanup and error handling:
export async function createDisposableWebSocketServer() {
const port = await getAvailablePort();
const server = new WebSocketServer({ port });
const sockets = new Set<WebSocket>();
server.on('connection', (conn) => {
sockets.add(conn);
- conn.once('close', () => sockets.delete(conn));
+ const cleanup = () => sockets.delete(conn);
+ conn.once('close', cleanup);
+ conn.once('error', cleanup);
});
const url = `ws://localhost:${port}`;
return {
url,
server,
[DisposableSymbols.asyncDispose]() {
- for (const socket of sockets) {
- socket.close(1001, 'Going Away');
- }
+ const closePromises = Array.from(sockets).map(
+ socket =>
+ new Promise<void>((resolve) => {
+ socket.once('close', () => resolve());
+ socket.close(1001, 'Going Away');
+ })
+ );
+
+ return Promise.all(closePromises)
+ .then(() =>
+ new Promise<void>((resolve, reject) => {
+ server.close((err) => {
+ if (err) {
+ reject(err);
+ } else {
+ resolve();
+ }
+ });
+ })
+ );
- return new Promise<void>((resolve, reject) => {
- server.close((err) => {
- if (err) {
- reject(err);
- } else {
- resolve();
- }
- });
- });
},
};
}The improvements include:
- Handle WebSocket errors to ensure proper cleanup
- Wait for all connections to close before closing the server
- Use Promise.all to handle multiple connection closures in parallel
📝 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 async function createDisposableWebSocketServer() { | |
| const port = await getAvailablePort(); | |
| const server = new WebSocketServer({ port }); | |
| const sockets = new Set<WebSocket>(); | |
| server.on('connection', (conn) => { | |
| sockets.add(conn); | |
| conn.once('close', () => sockets.delete(conn)); | |
| }); | |
| const url = `ws://localhost:${port}`; | |
| return { | |
| url, | |
| server, | |
| [DisposableSymbols.asyncDispose]() { | |
| for (const socket of sockets) { | |
| socket.close(1001, 'Going Away'); | |
| } | |
| return new Promise<void>((resolve, reject) => { | |
| server.close((err) => { | |
| if (err) { | |
| reject(err); | |
| } else { | |
| resolve(); | |
| } | |
| }); | |
| }); | |
| }, | |
| }; | |
| } | |
| export async function createDisposableWebSocketServer() { | |
| const port = await getAvailablePort(); | |
| const server = new WebSocketServer({ port }); | |
| const sockets = new Set<WebSocket>(); | |
| server.on('connection', (conn) => { | |
| sockets.add(conn); | |
| const cleanup = () => sockets.delete(conn); | |
| conn.once('close', cleanup); | |
| conn.once('error', cleanup); | |
| }); | |
| const url = `ws://localhost:${port}`; | |
| return { | |
| url, | |
| server, | |
| [DisposableSymbols.asyncDispose]() { | |
| const closePromises = Array.from(sockets).map( | |
| socket => | |
| new Promise<void>((resolve) => { | |
| socket.once('close', () => resolve()); | |
| socket.close(1001, 'Going Away'); | |
| }) | |
| ); | |
| return Promise.all(closePromises) | |
| .then(() => | |
| new Promise<void>((resolve, reject) => { | |
| server.close((err) => { | |
| if (err) { | |
| reject(err); | |
| } else { | |
| resolve(); | |
| } | |
| }); | |
| }) | |
| ); | |
| }, | |
| }; | |
| } |
Important
This introduces a breaking change and will request major bump to affected packages.
Ref GW-49
TODO