-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(executor-graphql-ws): respect the given lazy
and lazyCloseTimeout
options
#591
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes address a regression in the Changes
Possibly related PRs
Tip 🌐 Web search-backed reviews and chat
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🪧 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 (
|
🚀 Snapshot Release (
|
Package | Version | Info |
---|---|---|
@graphql-tools/executor-graphql-ws |
2.0.1-alpha-428d839c1f4eef2b452ff9e7dc9a82dcc5e8d0fd |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/gateway |
1.9.3-alpha-428d839c1f4eef2b452ff9e7dc9a82dcc5e8d0fd |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-ws |
1.0.1-alpha-428d839c1f4eef2b452ff9e7dc9a82dcc5e8d0fd |
npm ↗︎ unpkg ↗︎ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/executors/graphql-ws/src/index.ts (1)
76-81
: Simplify the boolean logic for thelazy
option.The ternary operator for the
lazy
option can be simplified.Apply this diff to simplify the boolean logic:
- lazy: clientOptionsOrClient.lazy === false ? false : true, + lazy: clientOptionsOrClient.lazy !== false,🧰 Tools
🪛 Biome (1.9.4)
[error] 79-79: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
.changeset/twenty-brooms-rush.md (1)
1-6
: Improve the grammar in the changeset description.The description has minor grammatical issues.
Apply this diff to improve the grammar:
-Fix the regression preventing user to pass custom `lazy` and `lazyCloseTimeout` options +Fix the regression preventing users from passing custom `lazy` and `lazyCloseTimeout` options🧰 Tools
🪛 LanguageTool
[uncategorized] ~5-~5: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...atch --- Fix the regression preventing user to pass customlazy
and `lazyCloseTim...(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
[uncategorized] ~5-~5: This verb may not be in the correct form. Consider using a different form for this context.
Context: ... Fix the regression preventing user to pass customlazy
andlazyCloseTimeout
op...(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/twenty-brooms-rush.md
(1 hunks)packages/executors/graphql-ws/src/index.ts
(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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.
🪛 LanguageTool
.changeset/twenty-brooms-rush.md
[uncategorized] ~5-~5: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...atch --- Fix the regression preventing user to pass custom lazy
and `lazyCloseTim...
(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
[uncategorized] ~5-~5: This verb may not be in the correct form. Consider using a different form for this context.
Context: ... Fix the regression preventing user to pass custom lazy
and lazyCloseTimeout
op...
(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
🪛 Biome (1.9.4)
packages/executors/graphql-ws/src/index.ts
[error] 79-79: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
⏰ Context from checks skipped due to timeout of 90000ms (44)
- GitHub Check: Examples / Convert operation-field-permissions
- GitHub Check: Examples / Convert type-merging-batching
- GitHub Check: Examples / Convert subscriptions-with-transforms
- GitHub Check: Examples / Convert programmatic-batching
- GitHub Check: Examples / Convert openapi-subscriptions
- GitHub Check: Examples / Convert openapi-javascript-wiki
- GitHub Check: Examples / Convert openapi-arg-rename
- GitHub Check: Examples / Convert openapi-additional-resolvers
- GitHub Check: Examples / Convert json-schema-subscriptions
- GitHub Check: Examples / Convert interface-additional-resolvers
- GitHub Check: Examples / Convert hmac-auth-https
- GitHub Check: Examples / Convert federation-subscriptions-passthrough
- GitHub Check: Examples / Convert apq-subgraphs
- GitHub Check: Examples / Convert file-upload
- GitHub Check: Examples / Convert federation-mixed
- GitHub Check: Examples / Convert federation-example
- GitHub Check: Examples / Convert extra-fields
- 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: Bun Docker image
- GitHub Check: Binary built on macos-13
- GitHub Check: Node Docker image
- GitHub Check: Binary built on ubuntu-latest
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: Bundle
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Build
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: Leaks / Node v22
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: Benchmark / node / 10 items
- GitHub Check: Leaks / Node v20
- GitHub Check: Unit / Bun
- GitHub Check: Leaks / Node v18
🔇 Additional comments (3)
packages/executors/graphql-ws/src/index.ts (3)
12-13
: LGTM!The added type imports enhance type safety and properly support the refactored code.
18-18
: LGTM!The explicit function signature for the
88-89
: LGTM!The client creation is properly encapsulated with the refactored options.
🚀 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 (Bun Docker Image)The latest changes of this PR are available as image on GitHub Container Registry (based on the declared
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/twelve-ghosts-smoke.md (1)
1-6
: Enhance the changeset description.While the current description explains the optimization, it would be helpful to mention that this fixes a regression where
lazy
andlazyCloseTimeout
options were not being respected.--- '@graphql-mesh/transport-ws': patch --- -Avoid having an extra Client instantiation in the transport, and use the one in the executor +Fix regression where `lazy` and `lazyCloseTimeout` options were not being respected. This change also optimizes Client instantiation by reusing the executor's Client instance instead of creating an additional one in the transport layer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/twelve-ghosts-smoke.md
(1 hunks).changeset/twenty-brooms-rush.md
(1 hunks)packages/executors/graphql-ws/src/index.ts
(3 hunks)packages/transports/ws/src/index.ts
(3 hunks)packages/transports/ws/tests/ws.spec.ts
(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/twenty-brooms-rush.md
🧰 Additional context used
📓 Path-based instructions (3)
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.
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/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.
⏰ Context from checks skipped due to timeout of 90000ms (29)
- GitHub Check: Binary built on ubuntu-24.04-arm
- GitHub Check: Binary built on windows-latest
- GitHub Check: Bun Docker image
- GitHub Check: Binary built on macos-14
- GitHub Check: Node Docker image
- GitHub Check: Binary built on macos-13
- GitHub Check: Binary built on ubuntu-latest
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: Unit / Bun
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Leaks / Node v22
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: Leaks / Node v20
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: Unit / Node v18
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: Leaks / Node v18
- GitHub Check: Bundle
- GitHub Check: Benchmark / node / 10 items
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build
🔇 Additional comments (7)
packages/transports/ws/src/index.ts (3)
37-41
: LGTM! Well-documented internal API.The
onClient
parameter is properly documented with@internal
JSDoc comment to indicate it's not part of the public API.
80-116
: LGTM! Comprehensive WebSocket lifecycle logging.The implementation properly handles all WebSocket lifecycle events with detailed logging, and the
closed
event correctly cleans up the executor map to prevent memory leaks.
83-85
: Verify default values for lazy options.The code sets default values for
lazy: true
andlazyCloseTimeout: 3_000
. These values should be documented in the README to help users understand the default behavior.packages/executors/graphql-ws/src/index.ts (3)
28-28
: LGTM! Improved type safety.The print function type is now properly defined with explicit parameter and return types.
59-80
: LGTM! Well-documented internal APIs.The
on
andonClient
options are properly documented with@internal
JSDoc comments.
112-113
: LGTM! Fixed regression with lazy options.The code now properly respects the
lazy
andlazyCloseTimeout
options:
lazy
defaults totrue
unless explicitly set tofalse
lazyCloseTimeout
defaults to0
if not providedpackages/transports/ws/tests/ws.spec.ts (1)
224-231
: LGTM! Comprehensive WebSocket lifecycle testing.The test properly validates WebSocket lifecycle events using the new
onClient
callback.
Fixes the regression introduced in #481