Hive Gateway Driver for NestJS #667
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe pull request records a major version update for the Changes
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches
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. 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (2)
packages/nestjs/src/index.ts (2)
1-319: 🧹 Nitpick (assertive)Ensure a corresponding changeset is present for this new feature, and link a Linear issue.
Since this is a new feature in the
@graphql-hive/nestjspackage, it should include a changeset with a minor semver bump (rather than patch). Additionally, we do not see any mention of a "GW-*" Linear issue in the PR description. Please link the relevant Linear ticket or create one if none exists.Would you like me to open a follow-up issue or provide a script to confirm the presence of a “GW-*” reference in the PR description?
🧰 Tools
🪛 Biome (1.9.4)
[error] 48-50: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 28-28: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 243-243: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
[error] 245-245: Use Number.POSITIVE_INFINITY instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.POSITIVE_INFINITY instead.(lint/style/useNumberNamespace)
🪛 GitHub Check: Types
[failure] 85-85:
Argument of type 'boolean | undefined' is not assignable to parameter of type 'boolean'.
1-319: 🧹 Nitpick (assertive)Consider adding documentation in the
consoleproject.Because a new feature is introduced, it’s typically required to add or update relevant documentation in the
graphql-hive/consolerepository. This ensures end users understand how to configure and use the Hive Gateway Driver for NestJS.Shall I open a new issue in
graphql-hive/consoleto track adding documentation for the Hive Gateway Driver for NestJS?🧰 Tools
🪛 Biome (1.9.4)
[error] 48-50: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 28-28: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 243-243: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
[error] 245-245: Use Number.POSITIVE_INFINITY instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.POSITIVE_INFINITY instead.(lint/style/useNumberNamespace)
🪛 GitHub Check: Types
[failure] 85-85:
Argument of type 'boolean | undefined' is not assignable to parameter of type 'boolean'.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
.changeset/sixty-camels-design.md(1 hunks)e2e/nestjs/nest-cli.json(1 hunks)e2e/nestjs/nestjs.e2e.ts(1 hunks)e2e/nestjs/package.json(1 hunks)e2e/nestjs/src/app.module.ts(1 hunks)e2e/nestjs/src/app.service.ts(1 hunks)e2e/nestjs/src/main.ts(1 hunks)e2e/nestjs/tsconfig.build.json(1 hunks)e2e/nestjs/tsconfig.json(1 hunks)packages/nestjs/package.json(1 hunks)packages/nestjs/src/index.ts(1 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
`e2e/**`: This directory includes end-to-end tests for the g...
e2e/**: This directory includes end-to-end tests for the gateway.
Theexamplesdirectory is generated based on the code in this directory.
e2e/nestjs/tsconfig.build.jsone2e/nestjs/nest-cli.jsone2e/nestjs/package.jsone2e/nestjs/src/app.service.tse2e/nestjs/src/main.tse2e/nestjs/tsconfig.jsone2e/nestjs/src/app.module.tse2e/nestjs/nestjs.e2e.ts
`**`: For all PRs, we would like to verify that a Linear iss...
**: 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
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
e2e/nestjs/tsconfig.build.jsone2e/nestjs/nest-cli.jsone2e/nestjs/package.jsone2e/nestjs/src/app.service.tse2e/nestjs/src/main.tstsconfig.jsone2e/nestjs/tsconfig.jsone2e/nestjs/src/app.module.tse2e/nestjs/nestjs.e2e.tspackages/nestjs/src/index.tspackages/nestjs/package.json
`packages/**`: In this directory we keep all packages releva...
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 changesetwith a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminorsemver change. When a bug is fixed,patchneeds to be used. Themajorbump is used for a PR that has breaking changes.
packages/nestjs/src/index.tspackages/nestjs/package.json
🪛 GitHub Check: Types
e2e/nestjs/nestjs.e2e.ts
[failure] 16-16:
'nest' is declared but its value is never read.
[failure] 16-16:
'waitForExit' is declared but its value is never read.
packages/nestjs/src/index.ts
[failure] 85-85:
Argument of type 'boolean | undefined' is not assignable to parameter of type 'boolean'.
🪛 Biome (1.9.4)
packages/nestjs/src/index.ts
[error] 48-50: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 28-28: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 243-243: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
[error] 245-245: Use Number.POSITIVE_INFINITY instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.POSITIVE_INFINITY instead.
(lint/style/useNumberNamespace)
⏰ Context from checks skipped due to timeout of 90000ms (27)
- 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: Leaks / Node v23
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: Leaks / Node v22
- GitHub Check: Leaks / Node v20
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Leaks / Node v18
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: Unit / Bun
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Build
- GitHub Check: Unit / Node v23
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: Bundle
- GitHub Check: Unit / Node v22
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: Unit / Node v20
- GitHub Check: Benchmark / node / 10 items
- GitHub Check: Unit / Node v18
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
e2e/nestjs/src/app.service.ts (1)
3-8: Implementation looks good.The
getHellomethod is straightforward and relies on NestJS dependency injection. Consider adding a unit test for this service or expanding the content to reflect real-world usage in an example if needed.e2e/nestjs/src/main.ts (1)
4-11: Bootstrap logic is clear and standard.This entry point follows NestJS conventions. No issues found with the error handling approach, although you could optionally log more contextual errors (e.g., environment details) if needed.
e2e/nestjs/src/app.module.ts (1)
11-13:❓ Verification inconclusive
Consider validating the supergraph path.
The supergraph path is loaded from an environment variable with a fallback, but there's no validation to ensure the file exists. Consider adding validation during module initialization.
🏁 Script executed:
#!/bin/bash # Check if the default supergraph file exists in the e2e/nestjs directory fd -t f "supergraph.graphql" "e2e/nestjs"Length of output: 39
Action Required: Validate the Supergraph File Existence During Module Initialization
The current code sets the supergraph path from an environment variable or falls back to the relative path (
'./supergraph.graphql'), but there’s no runtime check to confirm that the file exists. Automated verification usingfdin thee2e/nestjsdirectory didn’t return any results, so please manually verify whether the file exists in the expected location. If it is missing, adding a file existence check (for example, using Node’sfs.existsSyncduring module initialization) would help avoid runtime errors.
- Ensure that if
process.env['SUPERGRAPH']is not provided, the module checks that./supergraph.graphqlexists.- If the file is not found, consider logging an error message or throwing an exception during startup.
e2e/nestjs/tsconfig.build.json (1)
1-4: LGTM!The TypeScript build configuration follows best practices by extending the base config and properly excluding test files and build artifacts.
e2e/nestjs/nest-cli.json (1)
1-8: LGTM! Standard NestJS CLI configuration.The configuration follows NestJS best practices with proper schema validation and compiler options.
packages/nestjs/package.json (1)
2-4: Update version from 0.0.0 before release.The version 0.0.0 indicates the package is not ready for release. Update to an appropriate version (e.g., 0.1.0 for initial release).
tsconfig.json (1)
63-66: LGTM! Path mappings are correctly configured.The new path mapping for
@graphql-hive/nestjsand reformatted mapping for@graphql-hive/logger-winstonare properly configured.
ardatan
force-pushed
the
nestjs-driver
branch
from
bf73461 to
ca8c777
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
.changeset/sixty-camels-design.md(1 hunks)e2e/nestjs/nest-cli.json(1 hunks)e2e/nestjs/nestjs.e2e.ts(1 hunks)e2e/nestjs/package.json(1 hunks)e2e/nestjs/src/app.module.ts(1 hunks)e2e/nestjs/src/app.service.ts(1 hunks)e2e/nestjs/src/main.ts(1 hunks)e2e/nestjs/tsconfig.build.json(1 hunks)e2e/nestjs/tsconfig.json(1 hunks)packages/nestjs/package.json(1 hunks)packages/nestjs/src/index.ts(1 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
`e2e/**`: This directory includes end-to-end tests for the g...
e2e/**: This directory includes end-to-end tests for the gateway.
Theexamplesdirectory is generated based on the code in this directory.
e2e/nestjs/tsconfig.build.jsone2e/nestjs/nest-cli.jsone2e/nestjs/src/app.service.tse2e/nestjs/tsconfig.jsone2e/nestjs/src/app.module.tse2e/nestjs/package.jsone2e/nestjs/nestjs.e2e.tse2e/nestjs/src/main.ts
`**`: For all PRs, we would like to verify that a Linear iss...
**: 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
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
e2e/nestjs/tsconfig.build.jsone2e/nestjs/nest-cli.jsone2e/nestjs/src/app.service.tse2e/nestjs/tsconfig.jsone2e/nestjs/src/app.module.tse2e/nestjs/package.jsontsconfig.jsone2e/nestjs/nestjs.e2e.tspackages/nestjs/src/index.tspackages/nestjs/package.jsone2e/nestjs/src/main.ts
`packages/**`: In this directory we keep all packages releva...
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 changesetwith a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminorsemver change. When a bug is fixed,patchneeds to be used. Themajorbump is used for a PR that has breaking changes.
packages/nestjs/src/index.tspackages/nestjs/package.json
🪛 GitHub Check: Types
e2e/nestjs/nestjs.e2e.ts
[failure] 16-16:
'nest' is declared but its value is never read.
[failure] 16-16:
'waitForExit' is declared but its value is never read.
packages/nestjs/src/index.ts
[failure] 85-85:
Argument of type 'boolean | undefined' is not assignable to parameter of type 'boolean'.
🪛 Biome (1.9.4)
packages/nestjs/src/index.ts
[error] 48-50: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 28-28: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 243-243: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
[error] 245-245: Use Number.POSITIVE_INFINITY instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.POSITIVE_INFINITY instead.
(lint/style/useNumberNamespace)
⏰ Context from checks skipped due to timeout of 90000ms (27)
- 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 / Node v23
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: Unit / Node v22
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: Leaks / Node v23
- GitHub Check: Unit / Node v20
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: Leaks / Node v22
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Bundle
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: Unit / Node v18
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: Build
- GitHub Check: Leaks / Node v20
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: Unit / Bun
- GitHub Check: Benchmark / node / 10 items
- GitHub Check: Leaks / Node v18
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (16)
packages/nestjs/src/index.ts (7)
1-319: Ensure a corresponding changeset file for the new package code.Since this file is part of
packages/, a changeset file is required to describe the new functionality and determine the correct version bump (likely aminorbump). Please create one withyarn changesetif not already present in this PR.🧰 Tools
🪛 Biome (1.9.4)
[error] 48-50: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 28-28: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 243-243: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
[error] 245-245: Use Number.POSITIVE_INFINITY instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.POSITIVE_INFINITY instead.(lint/style/useNumberNamespace)
🪛 GitHub Check: Types
[failure] 85-85:
Argument of type 'boolean | undefined' is not assignable to parameter of type 'boolean'.
28-28: Avoid using '{}' as a type.This is the same feedback provided previously. Replace
'{}'withRecord<string, unknown>or a more precise shape to avoid inadvertently allowing all non-nullish values.🧰 Tools
🪛 Biome (1.9.4)
[error] 28-28: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
48-50: Remove the unnecessary constructor.This constructor calls
super()but otherwise doesn't add any logic. It can be safely removed for cleaner code.🧰 Tools
🪛 Biome (1.9.4)
[error] 48-50: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
52-195: Implementation of thestartmethod looks well-structured.The method properly sets up additional type definitions and resolvers, initializes context plugins, handles multiple ways of configuring subscriptions, and gracefully integrates with Express or Fastify. Good job!
🧰 Tools
🪛 GitHub Check: Types
[failure] 85-85:
Argument of type 'boolean | undefined' is not assignable to parameter of type 'boolean'.
85-85: Fix the type ofoptions.debug.Coercing
options.debugto a boolean (e.g.,!!options.debug) or defaulting it tofalsewill avoid type errors where a strictly boolean parameter is expected.🧰 Tools
🪛 GitHub Check: Types
[failure] 85-85:
Argument of type 'boolean | undefined' is not assignable to parameter of type 'boolean'.
243-243: Avoid reassigning a function parameter.Use a local variable instead of reassigning
argsto improve clarity and ensure the original reference remains intact.🧰 Tools
🪛 Biome (1.9.4)
[error] 243-243: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
245-245: Use Number.POSITIVE_INFINITY instead of Infinity.ES2015 encourages using
Number.POSITIVE_INFINITYfor consistency across the codebase.🧰 Tools
🪛 Biome (1.9.4)
[error] 245-245: Use Number.POSITIVE_INFINITY instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.POSITIVE_INFINITY instead.(lint/style/useNumberNamespace)
e2e/nestjs/nestjs.e2e.ts (2)
16-22: Add cleanup after test completion.The test spawns a NestJS server but doesn't clean it up after the test. Consider using the
waitForExitfunction in anafterAllorafterEachblock to ensure proper cleanup.🧰 Tools
🪛 GitHub Check: Types
[failure] 16-16:
'nest' is declared but its value is never read.
[failure] 16-16:
'waitForExit' is declared but its value is never read.
16-16: Address unused variables from destructuring.The
nestvariable is declared but never used. Consider removing it if not needed.🧰 Tools
🪛 GitHub Check: Types
[failure] 16-16:
'nest' is declared but its value is never read.
[failure] 16-16:
'waitForExit' is declared but its value is never read..changeset/sixty-camels-design.md (1)
5-5: Enhance the changeset description.The current description is too brief. Consider adding more details about:
- What the NestJS driver does
- Key features or capabilities
- Any breaking changes or important notes
e2e/nestjs/tsconfig.build.json (1)
1-4: LGTM!The TypeScript build configuration follows best practices by:
- Extending the base configuration
- Properly excluding test files and build artifacts
e2e/nestjs/nest-cli.json (1)
1-8: LGTM!The NestJS CLI configuration follows best practices with proper schema validation and compiler options.
e2e/nestjs/tsconfig.json (1)
17-19: Consider enabling strict TypeScript checks.Disabled strict checks could allow type-related bugs to slip through. Consider enabling them for better type safety.
e2e/nestjs/package.json (1)
13-21: Consider pinning dependency versions for stability.Using caret ranges for critical dependencies could lead to unexpected behavior in CI.
packages/nestjs/package.json (1)
1-61: Missing changeset file for package changes.According to coding guidelines, a changeset file created using
yarn changesetis required for package changes.tsconfig.json (1)
63-66: LGTM!The path mappings follow the existing pattern and are correctly configured.
🚀 Snapshot Release (
|
| Package | Version | Info |
|---|---|---|
@graphql-tools/batch-delegate |
9.0.32-alpha-d737d3cea3351c87dca61c6664418d6565d462a6 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/batch-execute |
9.0.13-alpha-d737d3cea3351c87dca61c6664418d6565d462a6 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/delegate |
10.2.14-alpha-d737d3cea3351c87dca61c6664418d6565d462a6 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-graphql-ws |
2.0.4-alpha-d737d3cea3351c87dca61c6664418d6565d462a6 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-http |
1.2.9-alpha-d737d3cea3351c87dca61c6664418d6565d462a6 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/federation |
3.1.5-alpha-d737d3cea3351c87dca61c6664418d6565d462a6 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/fusion-runtime |
0.11.3-alpha-d737d3cea3351c87dca61c6664418d6565d462a6 |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/gateway |
1.11.0-alpha-d737d3cea3351c87dca61c6664418d6565d462a6 |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/logger-winston |
1.0.2-alpha-d737d3cea3351c87dca61c6664418d6565d462a6 |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/nestjs |
1.0.0-alpha-d737d3cea3351c87dca61c6664418d6565d462a6 |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/plugin-aws-sigv4 |
1.0.0-alpha-d737d3cea3351c87dca61c6664418d6565d462a6 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/hmac-upstream-signature |
1.2.22-alpha-d737d3cea3351c87dca61c6664418d6565d462a6 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-opentelemetry |
1.3.45-alpha-d737d3cea3351c87dca61c6664418d6565d462a6 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-prometheus |
1.3.33-alpha-d737d3cea3351c87dca61c6664418d6565d462a6 |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/gateway-runtime |
1.5.0-alpha-d737d3cea3351c87dca61c6664418d6565d462a6 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/stitch |
9.4.19-alpha-d737d3cea3351c87dca61c6664418d6565d462a6 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/stitching-directives |
3.1.29-alpha-d737d3cea3351c87dca61c6664418d6565d462a6 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-http |
0.6.35-alpha-d737d3cea3351c87dca61c6664418d6565d462a6 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-http-callback |
0.5.22-alpha-d737d3cea3351c87dca61c6664418d6565d462a6 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-ws |
1.0.5-alpha-d737d3cea3351c87dca61c6664418d6565d462a6 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/wrap |
10.0.32-alpha-d737d3cea3351c87dca61c6664418d6565d462a6 |
npm ↗︎ unpkg ↗︎ |
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Binary for
|
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
e2e/nestjs/src/main.ts (1)
1-19: 🧹 Nitpick (assertive)Graceful shutdown and port usage are well-handled.
You might also consider handlingSIGINTin addition toSIGTERMif you want consistent local termination handling. Otherwise, this approach appears solid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
.changeset/sixty-camels-design.md(1 hunks)e2e/nestjs/.gitignore(1 hunks)e2e/nestjs/nestjs.e2e.ts(1 hunks)e2e/nestjs/package.json(1 hunks)e2e/nestjs/src/main.ts(1 hunks)e2e/nestjs/tsconfig.json(1 hunks)packages/nestjs/package.json(1 hunks)packages/nestjs/src/index.ts(1 hunks)tsconfig.json(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
`e2e/**`: This directory includes end-to-end tests for the g...
e2e/**: This directory includes end-to-end tests for the gateway.
Theexamplesdirectory is generated based on the code in this directory.
e2e/nestjs/package.jsone2e/nestjs/src/main.tse2e/nestjs/tsconfig.jsone2e/nestjs/nestjs.e2e.ts
`**`: For all PRs, we would like to verify that a Linear iss...
**: 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
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
e2e/nestjs/package.jsone2e/nestjs/src/main.tse2e/nestjs/tsconfig.jsontsconfig.jsone2e/nestjs/nestjs.e2e.tspackages/nestjs/src/index.tspackages/nestjs/package.json
`packages/**`: In this directory we keep all packages releva...
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 changesetwith a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminorsemver change. When a bug is fixed,patchneeds to be used. Themajorbump is used for a PR that has breaking changes.
packages/nestjs/src/index.tspackages/nestjs/package.json
🧠 Learnings (1)
e2e/nestjs/package.json (1)
Learnt from: ardatan
PR: graphql-hive/gateway#667
File: e2e/nestjs/package.json:13-21
Timestamp: 2025-02-14T13:52:33.387Z
Learning: In example and e2e test projects within the repository, using caret ranges (^) for dependencies is preferred to demonstrate compatibility across different versions.
⏰ Context from checks skipped due to timeout of 90000ms (31)
- GitHub Check: Examples / Inspect head commit
- 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: Unit / Bun
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Unit / Node v23
- GitHub Check: Leaks / Node v23
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Unit / Node v22
- GitHub Check: Leaks / Node v22
- GitHub Check: Benchmark / bun / 1000 items
- 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: Build
- GitHub Check: Leaks / Node v20
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: Bundle
- GitHub Check: Benchmark / node / 10 items
- GitHub Check: Leaks / Node v18
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (15)
packages/nestjs/src/index.ts (6)
1-27: Overall imports look good.
No immediate concerns with the imports or type references. Everything appears consistent with NestJS and the Hive Gateway runtime packages.
42-45: File is inpackages/**: verify presence of a changeset.
This file introduces a new driver in thepackages/nestjsdirectory. Please confirm that a.changesetfile describing these changes has been added, as per the repository guidelines.
194-197: Graceful stopping logic looks appropriate.
This ensures subscription services and the gateway runtime are properly disposed of. Nicely done.
199-225: Express & Fastify registration are straightforward.
The routing logic for Express and Fastify is correct. The approach to pass the request/response intohandleNodeRequestAndResponseis well-structured.
228-315: Logger adapter is well-implemented.
Your approach to flatten arguments and differentiate between string vs. object outputs is a clear pattern for structured logging.
317-323: Helper function for truthy checks is well thought out.
This utility method covers common truthy variants effectively.e2e/nestjs/nestjs.e2e.ts (1)
13-35: Add cleanup after test completion.
This test spawns a NestJS process but does not shut it down after the test, potentially leaving behind a running server. Consider using a teardown hook or waiting for exit.afterAll(async () => { - // ... + // Ensure server is closed after test completion: + await waitForExit(); });e2e/nestjs/.gitignore (1)
1-1: LGTM!Appropriate configuration to ignore build artifacts.
.changeset/sixty-camels-design.md (1)
5-7: Enhance the changeset description.The current description is too brief. Consider adding more details about:
- What the NestJS driver does
- Key features or capabilities
- Any breaking changes or important notes
e2e/nestjs/tsconfig.json (1)
1-21: LGTM!The TypeScript configuration follows best practices with strict type checking enabled and appropriate settings for a NestJS application.
e2e/nestjs/package.json (1)
13-20: LGTM!Dependencies use caret ranges, which is appropriate for example projects to demonstrate compatibility across versions.
packages/nestjs/package.json (1)
1-64: Action Required: Missing Documentation for NestJS DriverThe verification script did not return any open PRs for documentation related to the NestJS driver in the graphql-hive/console repository. This confirms that no corresponding documentation PR is currently present.
#!/bin/bash # Search for documentation PR in graphql-hive/console repository gh search prs --repo graphql-hive/console --state open --json title,url \ --jq '.[] | select(.title | contains("NestJS") or contains("Nest.js") or contains("nest"))'tsconfig.json (3)
63-66: LGTM!The path mappings are correctly configured:
- Reformatted
@graphql-hive/logger-winstonfor consistency- Added new
@graphql-hive/nestjsmapping following the established pattern
87-89: LGTM!The exclude patterns are appropriate:
- Added trailing comma for better maintainability
- Excluded build artifacts (dist) and local/temporary files (ignored-hidden)
1-91:❓ Verification inconclusive
Documentation needed for the NestJS driver feature.
The PR description mentions that documentation is missing. Since this is a new feature (NestJS driver), documentation should be added to help users understand and implement it correctly.
Let's check if there's a documentation PR in the console repository:
Please create a follow-up issue to add documentation for the NestJS driver feature in the console repository.
🏁 Script executed:
#!/bin/bash # Search for PRs in graphql-hive/console repository that might contain NestJS documentation gh search prs --repo graphql-hive/console "nestjs in:title,body" --state all --json title,urlLength of output: 3412
Documentation missing for NestJS driver feature – please add it.
The PR description indicates that documentation is absent. Our initial attempt to locate any existing NestJS-related documentation in the console repository was inconclusive due to an invalid search parameter (using
--state all). It’s likely that no documentation has been added yet. Please verify manually if any NestJS documentation exists in the console repo and, if not, create a follow-up issue to add the necessary documentation for the NestJS driver feature.
🚀 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
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
e2e/nestjs/nestjs.e2e.ts(1 hunks)e2e/nestjs/package.json(1 hunks)e2e/nestjs/services/nestjs/app.module.ts(1 hunks)e2e/nestjs/services/nestjs/index.ts(1 hunks)internal/e2e/src/tenv.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`e2e/**`: This directory includes end-to-end tests for the g...
e2e/**: This directory includes end-to-end tests for the gateway.
Theexamplesdirectory is generated based on the code in this directory.
e2e/nestjs/package.jsone2e/nestjs/services/nestjs/index.tse2e/nestjs/services/nestjs/app.module.tse2e/nestjs/nestjs.e2e.ts
`**`: For all PRs, we would like to verify that a Linear iss...
**: 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
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
e2e/nestjs/package.jsone2e/nestjs/services/nestjs/index.tse2e/nestjs/services/nestjs/app.module.tsinternal/e2e/src/tenv.tse2e/nestjs/nestjs.e2e.ts
🧠 Learnings (1)
e2e/nestjs/package.json (1)
Learnt from: ardatan
PR: graphql-hive/gateway#667
File: e2e/nestjs/package.json:13-21
Timestamp: 2025-02-14T13:52:33.387Z
Learning: In example and e2e test projects within the repository, using caret ranges (^) for dependencies is preferred to demonstrate compatibility across different versions.
⏰ Context from checks skipped due to timeout of 90000ms (33)
- GitHub Check: Examples / Inspect head commit
- GitHub Check: Binary built on ubuntu-24.04-arm
- GitHub Check: Binary built on macos-14
- GitHub Check: Binary built on macos-13
- GitHub Check: Bun Docker image
- GitHub Check: Binary built on ubuntu-latest
- 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: Unit / Bun
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: Leaks / Node v23
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: Unit / Node v23
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: Leaks / Node v22
- GitHub Check: Unit / Node v22
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: Unit / Node v18
- 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: Leaks / Node v18
- GitHub Check: Bundle
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: Build
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Benchmark / node / 10 items
🔇 Additional comments (8)
e2e/nestjs/services/nestjs/app.module.ts (1)
1-20: LGTM!The module configuration is well-structured and correctly integrates the HiveGatewayDriver with NestJS. The supergraph configuration is properly handled using the Opts utility.
e2e/nestjs/services/nestjs/index.ts (1)
1-22: LGTM!The bootstrap implementation is robust with proper error handling and graceful shutdown support.
e2e/nestjs/nestjs.e2e.ts (2)
9-26: LGTM!The test implementation is well-structured and correctly validates the GraphQL functionality.
19-22: Add cleanup and address unused variables.
- Add cleanup after test completion to ensure proper resource management.
- The
nestvariable is declared but never used.Apply this diff to fix the issues:
bootstrap().catch((e) => { console.error(e); process.exit(1); }); + +afterEach(async () => { + await waitForExit(); +});And:
-const [nest, waitForExit] = await spawn('yarn nest', { +const [, waitForExit] = await spawn('yarn nest', {internal/e2e/src/tenv.ts (1)
687-707: LGTM!The environment variable handling is correctly implemented in both service and container methods.
e2e/nestjs/package.json (3)
1-4: LGTM!The package metadata is well-structured with appropriate name, version, and private flag for an e2e test package.
5-13: LGTM! Versioning strategy aligns with project preferences.The use of caret ranges for dependencies is appropriate here as this is an e2e test project meant to demonstrate compatibility across versions.
14-31: LGTM! Development dependencies are well configured.The devDependencies include all necessary tools for building, testing, and development with appropriate versions.
🚀 Snapshot Release (Binary for
|
ardatan
force-pushed
the
nestjs-driver
branch
from
7bef7e4 to
3f41d58
Compare
ardatan
force-pushed
the
nestjs-driver
branch
from
b49673d to
bc64bc9
Compare
dotansimha
left a comment
dotansimha
left a comment
There was a problem hiding this comment.
lgtm!
let's just make sure to add docs before releasing this one?
@EmrysMyrddin can you please also review?
ardatan
force-pushed
the
nestjs-driver
branch
3 times, most recently
from
4ecd48f to
df1c2b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/nestjs/src/index.ts (1)
28-30: 🧹 Nitpick (assertive)Consider stronger typing for the context generic.
Using
TContext extends Record<string, unknown>instead ofRecord<string, any>can help improve type safety and avoid potential pitfalls withany.-export type HiveGatewayDriverConfig< - TContext extends Record<string, any> = Record<string, any>, -> = GatewayConfig<TContext> & ... +export type HiveGatewayDriverConfig< + TContext extends Record<string, unknown> = Record<string, unknown>, +> = GatewayConfig<TContext> & ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
.changeset/sixty-camels-design.md(1 hunks)e2e/nestjs/nestjs.e2e.ts(1 hunks)e2e/nestjs/package.json(1 hunks)e2e/nestjs/services/nestjs.ts(1 hunks)internal/e2e/src/tenv.ts(2 hunks)packages/nestjs/src/index.ts(1 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
`**`: For all PRs, we would like to verify that a Linear iss...
**: 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
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
tsconfig.jsone2e/nestjs/package.jsone2e/nestjs/services/nestjs.tse2e/nestjs/nestjs.e2e.tspackages/nestjs/src/index.tsinternal/e2e/src/tenv.ts
`e2e/**`: This directory includes end-to-end tests for the g...
e2e/**: This directory includes end-to-end tests for the gateway.
Theexamplesdirectory is generated based on the code in this directory.
e2e/nestjs/package.jsone2e/nestjs/services/nestjs.tse2e/nestjs/nestjs.e2e.ts
`packages/**`: In this directory we keep all packages releva...
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 changesetwith a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminorsemver change. When a bug is fixed,patchneeds to be used. Themajorbump is used for a PR that has breaking changes.
packages/nestjs/src/index.ts
🧠 Learnings (1)
e2e/nestjs/package.json (1)
Learnt from: ardatan
PR: graphql-hive/gateway#667
File: e2e/nestjs/package.json:13-21
Timestamp: 2025-02-14T13:52:33.387Z
Learning: In example and e2e test projects within the repository, using caret ranges (^) for dependencies is preferred to demonstrate compatibility across different versions.
⏰ Context from checks skipped due to timeout of 90000ms (24)
- 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: E2E / Node 23 on Ubuntu
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Leaks / Node v23
- GitHub Check: Snapshot / snapshot
- GitHub Check: Bundle
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: Leaks / Node v22
- GitHub Check: Unit / Bun
- GitHub Check: Build
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: Leaks / Node v20
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: Leaks / Node v18
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: Benchmark / node / 10 items
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (11)
packages/nestjs/src/index.ts (3)
49-193: Ensure a corresponding changeset is included.A new driver is a significant enhancement. Please confirm a changeset file was created using
yarn changeset, describing this new feature with a suggested minor semver bump.
228-315: Adapter design is well-implemented.
No major concerns here; the structured logging approach is clear and flexible.
317-323:truthyhelper seems fine.
This utility is straightforward and consistent with typical environment variable checks.e2e/nestjs/services/nestjs.ts (1)
1-34: Overall implementation looks good.
This file straightforwardly sets up the NestJS server with the new driver. No fixes required.e2e/nestjs/nestjs.e2e.ts (1)
9-26: Add cleanup after the test completes.
This is a repeated concern from previous reviews. Make sure you stop the server or clean up resources after the test to avoid lingering processes.+ afterAll(async () => { + // or afterEach if needed + await waitForExit(); + });internal/e2e/src/tenv.ts (2)
686-729: LGTM! Environment variable support added to service function.The addition of the
envparameter to the service function is well-implemented, maintaining backward compatibility while enabling environment variable configuration for services.
730-962: LGTM! Environment variable support added to container function.The addition of the
envparameter with a default empty object is a good practice, maintaining backward compatibility while allowing container environment configuration.🧰 Tools
🪛 Biome (1.9.4)
[error] 822-823: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 826-827: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 835-835: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 839-839: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 826-826: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
[error] 839-839: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
.changeset/sixty-camels-design.md (2)
1-3: Update version bump type for new feature.According to the coding guidelines, new features should use a
minorversion bump instead ofpatch.--- -'@graphql-hive/nestjs': patch +'@graphql-hive/nestjs': minor ---
5-7: Enhance the changeset description.The current description is too brief. Consider adding more details about:
- What the NestJS driver does
- Key features or capabilities
- Any breaking changes or important notes
e2e/nestjs/package.json (1)
1-15: LGTM! Dependencies properly configured for e2e tests.The use of caret ranges (^) for dependencies is appropriate for example and e2e test projects, as it helps demonstrate compatibility across different versions.
tsconfig.json (1)
63-66: LGTM! Path mappings properly configured.The path mappings are correctly formatted and consistent with the project's style:
- Reformatted @graphql-hive/logger-winston for better readability
- Added new mapping for @graphql-hive/nestjs
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (31)
examples/apq-subgraphs/example.tar.gzis excluded by!**/*.gzexamples/apq-subgraphs/package-lock.jsonis excluded by!**/package-lock.jsonexamples/extra-fields/example.tar.gzis excluded by!**/*.gzexamples/extra-fields/package-lock.jsonis excluded by!**/package-lock.jsonexamples/federation-mixed/example.tar.gzis excluded by!**/*.gzexamples/federation-mixed/package-lock.jsonis excluded by!**/package-lock.jsonexamples/file-upload/example.tar.gzis excluded by!**/*.gzexamples/file-upload/package-lock.jsonis excluded by!**/package-lock.jsonexamples/hmac-auth-https/example.tar.gzis excluded by!**/*.gzexamples/hmac-auth-https/package-lock.jsonis excluded by!**/package-lock.jsonexamples/interface-additional-resolvers/example.tar.gzis excluded by!**/*.gzexamples/interface-additional-resolvers/package-lock.jsonis excluded by!**/package-lock.jsonexamples/json-schema-subscriptions/example.tar.gzis excluded by!**/*.gzexamples/json-schema-subscriptions/package-lock.jsonis excluded by!**/package-lock.jsonexamples/openapi-additional-resolvers/example.tar.gzis excluded by!**/*.gzexamples/openapi-additional-resolvers/package-lock.jsonis excluded by!**/package-lock.jsonexamples/openapi-arg-rename/example.tar.gzis excluded by!**/*.gzexamples/openapi-arg-rename/package-lock.jsonis excluded by!**/package-lock.jsonexamples/openapi-javascript-wiki/example.tar.gzis excluded by!**/*.gzexamples/openapi-javascript-wiki/package-lock.jsonis excluded by!**/package-lock.jsonexamples/openapi-subscriptions/example.tar.gzis excluded by!**/*.gzexamples/openapi-subscriptions/package-lock.jsonis excluded by!**/package-lock.jsonexamples/operation-field-permissions/example.tar.gzis excluded by!**/*.gzexamples/operation-field-permissions/package-lock.jsonis excluded by!**/package-lock.jsonexamples/programmatic-batching/example.tar.gzis excluded by!**/*.gzexamples/programmatic-batching/package-lock.jsonis excluded by!**/package-lock.jsonexamples/subscriptions-with-transforms/example.tar.gzis excluded by!**/*.gzexamples/subscriptions-with-transforms/package-lock.jsonis excluded by!**/package-lock.jsonexamples/type-merging-batching/example.tar.gzis excluded by!**/*.gzexamples/type-merging-batching/package-lock.jsonis excluded by!**/package-lock.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
.changeset/sixty-camels-design.md(1 hunks)e2e/nestjs/nestjs.e2e.ts(1 hunks)e2e/nestjs/package.json(1 hunks)e2e/nestjs/services/nestjs.ts(1 hunks)internal/e2e/src/tenv.ts(2 hunks)packages/nestjs/src/index.ts(1 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
`e2e/**`: This directory includes end-to-end tests for the g...
e2e/**: This directory includes end-to-end tests for the gateway.
Theexamplesdirectory is generated based on the code in this directory.
e2e/nestjs/package.jsone2e/nestjs/services/nestjs.tse2e/nestjs/nestjs.e2e.ts
`**`: For all PRs, we would like to verify that a Linear iss...
**: 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
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
e2e/nestjs/package.jsontsconfig.jsone2e/nestjs/services/nestjs.tse2e/nestjs/nestjs.e2e.tspackages/nestjs/src/index.tsinternal/e2e/src/tenv.ts
`packages/**`: In this directory we keep all packages releva...
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 changesetwith a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminorsemver change. When a bug is fixed,patchneeds to be used. Themajorbump is used for a PR that has breaking changes.
packages/nestjs/src/index.ts
🧠 Learnings (1)
e2e/nestjs/package.json (1)
Learnt from: ardatan
PR: graphql-hive/gateway#667
File: e2e/nestjs/package.json:13-21
Timestamp: 2025-02-14T13:52:33.387Z
Learning: In example and e2e test projects within the repository, using caret ranges (^) for dependencies is preferred to demonstrate compatibility across different versions.
⏰ Context from checks skipped due to timeout of 90000ms (23)
- GitHub Check: Unit / Bun
- 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: E2E / Node 23 on Ubuntu
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Leaks / Node v23
- GitHub Check: Leaks / Node v22
- GitHub Check: Leaks / Node v20
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Leaks / Node v18
- GitHub Check: Bundle
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Build
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: Benchmark / node / 10 items
- GitHub Check: Snapshot / snapshot
🔇 Additional comments (10)
packages/nestjs/src/index.ts (3)
1-7: Ensure a changeset for the new driver.You are adding a new feature under
packages/**but I don’t see a changeset file in the provided context. Please confirm that a changeset exists so that this feature is properly versioned and documented.
82-82: Verify the environment debug logic.You're falling back to
truthy(process.env['DEBUG']). Ensure this behaves as intended if theDEBUGvariable is undefined or set to a non-boolean-like string.Would you like a script to search for other references to
process.env.DEBUGthroughout the repository to confirm consistent usage?
228-315: Good job usingNumber.POSITIVE_INFINITYfor flattening.This approach is consistent with best practices and ensures deeply nested arrays get handled properly.
e2e/nestjs/services/nestjs.ts (1)
13-21: Validate the supergraph path or provide a fallback.If the
supergraphpath is invalid or missing, the application might fail at runtime. Consider verifying the path and providing a meaningful error or default.internal/e2e/src/tenv.ts (2)
694-694: LGTM! Environment variables support added to service method.The addition of the
envparameter to the service method is well-implemented, maintaining backward compatibility while enabling environment configuration.Also applies to: 706-706
733-733: LGTM! Environment variables support added to container method.The addition of the
envparameter with a default empty object is well-implemented, maintaining backward compatibility while enabling environment configuration..changeset/sixty-camels-design.md (2)
1-3: Update version bump type for new feature.According to the coding guidelines, new features should use a
minorversion bump instead ofpatch.
5-7: Enhance the changeset description.The current description is too brief. Consider adding more details about:
- What the NestJS driver does
- Key features or capabilities
- Any breaking changes or important notes
e2e/nestjs/package.json (1)
6-13: LGTM! Dependencies are properly configured.The dependencies use caret ranges which is the preferred approach for example and e2e test projects to demonstrate compatibility across versions.
tsconfig.json (1)
63-66: LGTM! Path mappings are properly configured.The changes maintain consistent formatting and correctly map the new NestJS package.
ardatan
force-pushed
the
nestjs-driver
branch
3 times, most recently
from
a985671 to
5de67e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
.changeset/sixty-camels-design.md(1 hunks)e2e/nestjs/nestjs.e2e.ts(1 hunks)e2e/nestjs/package.json(1 hunks)e2e/nestjs/services/nestjs.ts(1 hunks)internal/e2e/src/tenv.ts(2 hunks)packages/nestjs/package.json(1 hunks)packages/nestjs/src/index.ts(1 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
`e2e/**`: This directory includes end-to-end tests for the g...
e2e/**: This directory includes end-to-end tests for the gateway.
Theexamplesdirectory is generated based on the code in this directory.
e2e/nestjs/package.jsone2e/nestjs/services/nestjs.tse2e/nestjs/nestjs.e2e.ts
`**`: For all PRs, we would like to verify that a Linear iss...
**: 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
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
e2e/nestjs/package.jsone2e/nestjs/services/nestjs.tstsconfig.jsonpackages/nestjs/package.jsone2e/nestjs/nestjs.e2e.tspackages/nestjs/src/index.tsinternal/e2e/src/tenv.ts
`packages/**`: In this directory we keep all packages releva...
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 changesetwith a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminorsemver change. When a bug is fixed,patchneeds to be used. Themajorbump is used for a PR that has breaking changes.
packages/nestjs/package.jsonpackages/nestjs/src/index.ts
🧠 Learnings (1)
e2e/nestjs/package.json (1)
Learnt from: ardatan
PR: graphql-hive/gateway#667
File: e2e/nestjs/package.json:13-21
Timestamp: 2025-02-14T13:52:33.387Z
Learning: In example and e2e test projects within the repository, using caret ranges (^) for dependencies is preferred to demonstrate compatibility across different versions.
⏰ Context from checks skipped due to timeout of 90000ms (28)
- GitHub Check: Unit / Node v23
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: Unit / Node v22
- GitHub Check: Unit / Bun
- GitHub Check: Unit / Node v20
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: Unit / Node v18
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: Leaks / Node v23
- GitHub Check: Leaks / Node v22
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: Snapshot / snapshot
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: Build
- GitHub Check: Leaks / Node v20
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: Bundle
- GitHub Check: Leaks / Node v18
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Benchmark / node / 10 items
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
packages/nestjs/src/index.ts (4)
28-40: Use a more specific context type.Consider replacing
Record<string, any>withRecord<string, unknown>for stricter type safety.
42-45: Add documentation comments for HiveGatewayDriver.It is recommended to document core classes within the package to help users understand the new driver’s usage and configuration.
83-83: Debug parameter default logic looks good.Using
?? truthy(process.env['DEBUG'])properly handlesundefinedvalues and ensures a boolean is passed.
317-323: Consider broadening definition of truthy values.You may want to handle uppercase strings like
"YES","TRUE"or numeric"1.0"if needed for a more flexible interpretation.e2e/nestjs/services/nestjs.ts (1)
25-28: Startup logic is straightforward and readable.No concerns here. The error flow is already handled with
.catch(...).e2e/nestjs/nestjs.e2e.ts (1)
9-26: Add cleanup for the spawned server.Please stop the server after the test to avoid resource leaks.
it('executes the query', async () => { ... expect(received).toEqual(result); }); + afterAll(async () => { + await waitForExit(); // or the relevant teardown function + });internal/e2e/src/tenv.ts (1)
694-694: LGTM! Environment variable support added.The addition of the
envparameter to theservicemethod enhances configurability by allowing environment variables to be passed to services.Also applies to: 706-706
.changeset/sixty-camels-design.md (1)
1-3: Update version bump type for new feature.According to the coding guidelines, new features should use a
minorversion bump instead ofpatch.--- -'@graphql-hive/nestjs': patch +'@graphql-hive/nestjs': minor ---e2e/nestjs/package.json (1)
6-13: LGTM! Appropriate use of caret ranges.The use of caret ranges (
^) for dependencies is appropriate here as this is an e2e test project, demonstrating compatibility across different versions.packages/nestjs/package.json (1)
1-64:❓ Verification inconclusive
Documentation required for NestJS Gateway Driver.
The package configuration is well-structured. However, as this is a new feature, documentation should be added to the console repository.
Please create a follow-up issue in the
graphql-hive/consolerepository to add comprehensive documentation for the NestJS Gateway Driver, including:
- Installation and setup instructions
- Configuration options and examples
- Integration patterns with NestJS applications
- Best practices and common use cases
🏁 Script executed:
#!/bin/bash # Search for documentation PR in graphql-hive/console repository gh search prs --repo graphql-hive/console --state open --json title,url \ --jq '.[] | select(.title | contains("NestJS") or contains("Nest.js") or contains("nest"))'Length of output: 165
Documentation Required: NestJS Gateway Driver Follow-Up
The package configuration looks solid, but because this new feature is being added, we need comprehensive documentation in the console repository. Please verify if there is any ongoing documentation PR; if not, create a follow-up issue in the
graphql-hive/consolerepository that includes:
- Installation and setup instructions
- Detailed configuration options with examples
- Integration patterns with NestJS applications
- Best practices and common use cases
Manual verification is recommended since our automated search did not yield any documentation-related PRs in the console repository.
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (2)
.changeset/small-lies-beg.md (1)
1-6: 🧹 Nitpick (assertive)Enhance the changeset description.
While the changeset correctly specifies the patch version bump, consider adding:
- Brief usage examples of the exposed methods
- A note about the internal API status and potential breaking changes
--- '@graphql-hive/gateway': patch --- -Expose internal methods `getCacheInstanceFromConfig` and `getBuiltinPluginsFromConfig` +Expose internal methods `getCacheInstanceFromConfig` and `getBuiltinPluginsFromConfig`. + +Note: These are internal APIs that may have breaking changes in future versions. + +Example usage: +```ts +import { getCacheInstanceFromConfig, getBuiltinPluginsFromConfig } from '@graphql-hive/gateway'; + +// Initialize cache +const cache = await getCacheInstanceFromConfig(config, context); + +// Get builtin plugins +const plugins = await getBuiltinPluginsFromConfig(config, context); +```packages/nestjs/src/index.ts (1)
1-341: 🧹 Nitpick (assertive)Add documentation references for the new NestJS driver.
The PR objectives mention documentation is missing for this feature. Consider creating a follow-up issue or PR in the
graphql-hive/consolerepository to ensure the Hive Gateway Driver for NestJS is clearly documented.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
.changeset/small-lies-beg.md(1 hunks)packages/gateway/src/config.ts(2 hunks)packages/gateway/src/index.ts(1 hunks)packages/nestjs/package.json(1 hunks)packages/nestjs/src/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
`packages/**`: In this directory we keep all packages releva...
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 changesetwith a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminorsemver change. When a bug is fixed,patchneeds to be used. Themajorbump is used for a PR that has breaking changes.
packages/nestjs/package.jsonpackages/gateway/src/index.tspackages/gateway/src/config.tspackages/nestjs/src/index.ts
`**`: For all PRs, we would like to verify that a Linear iss...
**: 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
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/nestjs/package.jsonpackages/gateway/src/index.tspackages/gateway/src/config.tspackages/nestjs/src/index.ts
`packages/gateway/**`: The main runtime for the Hive gateway...
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.
packages/gateway/src/index.tspackages/gateway/src/config.ts
⏰ Context from checks skipped due to timeout of 90000ms (23)
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: Unit / Bun
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: Unit / Node v23
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: Leaks / Node v23
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: Leaks / Node v22
- GitHub Check: Snapshot / snapshot
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: Leaks / Node v20
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: Bundle
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: Leaks / Node v18
- GitHub Check: Benchmark / node / 10 items
- GitHub Check: E2E / Node 18 on Ubuntu
🔇 Additional comments (6)
packages/gateway/src/index.ts (1)
29-32:❓ Verification inconclusive
LGTM! Verify documentation coverage.
The new exports are properly documented in the changeset and marked as internal APIs.
Since this PR introduces new functionality for NestJS integration, please ensure documentation is added in the
graphql-hive/consolerepository. Let me check for related documentation PRs:
🏁 Script executed:
#!/bin/bash # Search for related documentation PRs in the console repository gh api graphql -f query=' query { search(query: "repo:graphql-hive/console NestJS in:title,body", type: ISSUE, first: 5) { nodes { ... on PullRequest { title url state } } } }'Length of output: 897
LGTM! Please verify the NestJS integration documentation.
The new exports (
getCacheInstanceFromConfigandgetBuiltinPluginsFromConfig) are properly documented in the changeset and marked as internal APIs. However, since this PR introduces new functionality for NestJS integration, please double-check that the corresponding documentation in thegraphql-hive/consolerepository is updated—or that a dedicated documentation PR is opened—so that the integration is clearly covered.packages/gateway/src/config.ts (1)
1-218: Verify documentation requirements.The PR implements GW-133 and includes proper changesets. However, documentation for the NestJS integration appears to be missing.
Please create a follow-up issue in the
graphql-hive/consolerepository to add documentation for the NestJS integration. The documentation should cover:
- Setup and configuration of the Hive Gateway Driver in NestJS applications
- Usage examples of the exposed internal methods
- Best practices and caveats
packages/nestjs/src/index.ts (4)
32-45: Use a more specific context type rather thanRecord<string, any>.Continuing from previous suggestions, to enhance type safety and clarity, consider changing
Record<string, any>to a more specific object type orRecord<string, unknown>if you need a generic fallback. This better documents and constrains the shape of your context.-export type HiveGatewayDriverConfig< - TContext extends Record<string, any> = Record<string, any>, -> = GatewayConfig<TContext> & +export type HiveGatewayDriverConfig< + TContext extends Record<string, unknown> = Record<string, unknown>, +> = GatewayConfig<TContext> &
48-50: Add doc comments for the newHiveGatewayDriverclass.To help developers understand how to configure and use this driver, add JSDoc or TSDoc comments detailing its capabilities, usage examples, and important configuration options.
211-242: Handle potential runtime errors with a try-catch.When calling
this._gatewayRuntime.handleNodeRequestAndResponse, consider wrapping the logic in a try-catch block and returning an appropriate HTTP error response. This ensures that unexpected runtime errors don’t break the entire Fastify request lifecycle.-all('*', async (req: FastifyRequest, reply: FastifyReply) => { - if (!this._gatewayRuntime) { - throw new Error('Hive Gateway is not initialized'); - } - const response = await this._gatewayRuntime.handleNodeRequestAndResponse(req, reply, { - req, - reply, - }); - ... - return reply; +all('*', async (req: FastifyRequest, reply: FastifyReply) => { + try { + if (!this._gatewayRuntime) { + throw new Error('Hive Gateway is not initialized'); + } + const response = await this._gatewayRuntime.handleNodeRequestAndResponse(req, reply, { + req, + reply, + }); + ... + return reply; + } catch (err) { + this.logger.error('Request handling failed', err); + reply.status(500).send({ error: 'Internal Server Error' }); + } });
334-340: Consider expanding thetruthyfunction to handle uppercase or numeric strings.For broader control over debug flags or environment variables, you might include
"TRUE","YES", or"1.0"etc. This was suggested before for comprehensive truth detection.
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
.changeset/small-lies-beg.md (1)
5-6: 🧹 Nitpick (assertive)Clarify Exposure of Internal Methods
The description clearly states the exposure of internal methods. Consider adding a brief note specifying that these methods are experimental or may be subject to change in future releases, to set proper expectations with developers using this API.
♻️ Duplicate comments (6)
packages/nestjs/src/index.ts (6)
33-35: 🛠️ Refactor suggestionUse a more specific type for the driver configuration context.
It's recommended to avoid
Record<string, any>in favor ofRecord<string, unknown>or an even more specific type that precisely describes the shape of the context. This prevents unintentional usage ofany.-export type HiveGatewayDriverConfig< - TContext extends Record<string, any> = Record<string, any>, -> = GatewayConfig<TContext> & +export type HiveGatewayDriverConfig< + TContext extends Record<string, unknown> = Record<string, unknown>, +> = GatewayConfig<TContext> &
48-51: 🧹 Nitpick (assertive)Add doc comments for your new HiveGatewayDriver class.
Consider adding high-level documentation for how to configure and use this new driver, including how it integrates with NestJS and the gateway features. This helps other developers quickly understand setup steps and benefits.
126-127: 🧹 Nitpick (assertive)Consider using NestJS HttpException for unsupported adapters.
Throwing a plain
Erroris perfectly valid in a library, but if you want to align with NestJS conventions, consider usingHttpExceptionor a more specialized NestJS exception for better integration with the framework’s error handling pipeline.-throw new Error(\`No support for current HttpAdapter: \${platformName}\`); +throw new HttpException( + \`No support for current HttpAdapter: \${platformName}\`, + HttpStatus.NOT_IMPLEMENTED +);
84-89: 🧹 Nitpick (assertive)Log a message when debug mode is enabled.
Currently, the debug mode is silently respected. Logging a quick note upon startup can inform users that debug logging is active and help in troubleshooting.
const logger = new NestJSLoggerAdapter( 'Hive Gateway', {}, new NestLogger('Hive Gateway'), options.debug ?? truthy(process.env['DEBUG']), ); +if (options.debug ?? truthy(process.env['DEBUG'])) { + logger.log('Debug mode is enabled for Hive Gateway'); +}
335-341: 🧹 Nitpick (assertive)Consider expanding the
truthyhelper for additional edge cases.You might want to handle uppercase strings like
"YES", numeric-like strings such as"1.0", or other patterns that a user might consider “truthy.”function truthy(val: unknown) { return ( val === true || val === 1 || - ['1', 't', 'true', 'y', 'yes'].includes(String(val)) + ['1', 't', 'true', 'y', 'yes', 'on'].includes(String(val.toLowerCase())) ); }
49-51: 🧹 Nitpick (assertive)Follow up with documentation in the
graphql-hive/consolerepository.The PR objectives mention that documentation is missing. For a newly introduced feature like this driver, a complementary entry in the
consoleproject is recommended so users can easily reference usage details.Would you like me to open a follow-up issue in
graphql-hive/consoleto track this?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
.changeset/@graphql-hive_gateway-712-dependencies.md(1 hunks).changeset/sixty-camels-design.md(1 hunks).changeset/small-lies-beg.md(1 hunks)e2e/nestjs/nestjs.e2e.ts(1 hunks)e2e/nestjs/package.json(1 hunks)e2e/nestjs/services/nestjs.ts(1 hunks)internal/e2e/src/tenv.ts(2 hunks)packages/gateway/src/config.ts(2 hunks)packages/gateway/src/index.ts(1 hunks)packages/nestjs/package.json(1 hunks)packages/nestjs/src/index.ts(1 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
`e2e/**`: This directory includes end-to-end tests for the g...
e2e/**: This directory includes end-to-end tests for the gateway.
Theexamplesdirectory is generated based on the code in this directory.
e2e/nestjs/package.jsone2e/nestjs/services/nestjs.tse2e/nestjs/nestjs.e2e.ts
`**`: For all PRs, we would like to verify that a Linear iss...
**: 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
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
e2e/nestjs/package.jsonpackages/gateway/src/index.tstsconfig.jsonpackages/gateway/src/config.tse2e/nestjs/services/nestjs.tspackages/nestjs/package.jsone2e/nestjs/nestjs.e2e.tspackages/nestjs/src/index.tsinternal/e2e/src/tenv.ts
`packages/**`: In this directory we keep all packages releva...
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 changesetwith a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminorsemver change. When a bug is fixed,patchneeds to be used. Themajorbump is used for a PR that has breaking changes.
packages/gateway/src/index.tspackages/gateway/src/config.tspackages/nestjs/package.jsonpackages/nestjs/src/index.ts
`packages/gateway/**`: The main runtime for the Hive gateway...
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.
packages/gateway/src/index.tspackages/gateway/src/config.ts
🧠 Learnings (2)
e2e/nestjs/package.json (1)
Learnt from: ardatan
PR: graphql-hive/gateway#667
File: e2e/nestjs/package.json:13-21
Timestamp: 2025-02-14T13:52:33.387Z
Learning: In example and e2e test projects within the repository, using caret ranges (^) for dependencies is preferred to demonstrate compatibility across different versions.
packages/nestjs/package.json (1)
Learnt from: ardatan
PR: graphql-hive/gateway#667
File: packages/nestjs/package.json:3-3
Timestamp: 2025-02-21T15:09:57.628Z
Learning: Version management in the repository is handled through changesets rather than direct modifications to package.json version fields.
⏰ Context from checks skipped due to timeout of 90000ms (32)
- GitHub Check: Binary built on ubuntu-24.04-arm
- GitHub Check: Binary built on macos-14
- GitHub Check: Binary built on macos-13
- GitHub Check: Bun Docker image
- GitHub Check: Binary built on ubuntu-latest
- 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: Unit / Node v23
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: Leaks / Node v23
- GitHub Check: Unit / Bun
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: Leaks / Node v22
- GitHub Check: Unit / Node v18
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: Types
- GitHub Check: Leaks / Node v20
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: Bundle
- GitHub Check: Leaks / Node v18
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: Benchmark / node / 10 items
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (13)
.changeset/sixty-camels-design.md (1)
1-3: Major Version Bump for @graphql-hive/nestjs
The changeset correctly updates the version to "major" for the new NestJS driver, which aligns with our decision to promote API stability..changeset/small-lies-beg.md (1)
1-3: Minor Version Update for @graphql-hive/gateway
The changeset updates the version to "minor" to reflect the exposure of additional internal methods. Please verify that exposinggetCacheInstanceFromConfigandgetBuiltinPluginsFromConfigdoes not introduce breaking changes that would warrant a more significant version bump.e2e/nestjs/package.json (1)
1-13: Verify Dependency Specifiers in e2e Package
The dependencies, including those for various NestJS modules, are specified using caret ranges—which is preferred for e2e projects to demonstrate compatibility across versions. Ensure that the"workspace:^"specifier for@graphql-hive/nestjsis intentional and compatible with your workspace tooling..changeset/@graphql-hive_gateway-712-dependencies.md (1)
1-3: Patch Update for @graphql-hive/gateway
The changeset sets the version bump to "patch" for the gateway package. Please review whether exposing internal methods has any potential impact that might justify a minor version bump instead.tsconfig.json (1)
63-66: Enhanced Path Mappings and New NestJS Entry.The reformatting and new mapping for
@graphql-hive/nestjsare correctly configured. However, as this PR completes task GW-133 and adds new functionality, documentation updates in the corresponding console project are still missing.e2e/nestjs/services/nestjs.ts (1)
13-21: Add corresponding documentation for the new NestJS driver.The PR objectives mention missing documentation. Since this file demonstrates new functionality, ensure a follow-up issue or PR is created in the
graphql-hive/consolerepository.packages/gateway/src/config.ts (1)
99-102: LGTM! Consider adding return type documentation.The internal API documentation is clear. Consider adding JSDoc for the return type and parameters.
Also applies to: 155-158
packages/nestjs/package.json (2)
1-37: LGTM! Package metadata and configuration are well-structured.The package metadata, exports configuration, and type definitions are properly set up. The Node.js version requirement is appropriate for the features used.
38-41: LGTM! Build scripts are correctly configured.The build process using pkgroll with clean-dist option and prepack hook is properly set up.
internal/e2e/src/tenv.ts (2)
687-696: LGTM! Environment variables support added to service options.The
envparameter is correctly added to the service options interface.
699-707: LGTM! Environment variables properly passed to spawn function.The
envparameter is correctly propagated to the spawn function.e2e/nestjs/nestjs.e2e.ts (1)
1-8: LGTM! Test setup is well-structured.The imports and test environment setup are properly configured.
packages/nestjs/src/index.ts (1)
1-342: Switch to a minor semver release for this new feature.The guidelines specify a
minorversion bump for new features. Ensure that any automatically generated or manually maintained changeset reflects aminorrelease.
| Hive Gateway Driver for NestJS; | ||
|
|
||
| [Learn more in the docs](https://the-guild.dev/graphql/hive/docs/gateway/deployment/node-frameworks/nestjs) No newline at end of file |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhance Description for Hive Gateway Driver
The brief description and documentation link provide a good start, but given that documentation is currently missing (as noted in PR objectives), consider expanding the description or opening a follow-up issue to add comprehensive documentation in the console project. Also, ensure the GW-133 ticket reference is clearly noted elsewhere in project management.
| dependencies updates: | ||
|
|
||
| - Updated dependency [`@graphql-mesh/cache-localforage@^0.104.0` ↗︎](https://www.npmjs.com/package/@graphql-mesh/cache-localforage/v/0.104.0) (from `^0.103.19`, in `dependencies`) | ||
| - Updated dependency [`@graphql-mesh/cache-upstash-redis@^0.0.6` ↗︎](https://www.npmjs.com/package/@graphql-mesh/cache-upstash-redis/v/0.0.6) (from `^0.0.5`, in `dependencies`) | ||
| - Updated dependency [`@graphql-mesh/plugin-mock@^0.104.0` ↗︎](https://www.npmjs.com/package/@graphql-mesh/plugin-mock/v/0.104.0) (from `^0.103.19`, in `dependencies`) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Document Dependency Updates
The updated dependency versions (with links to the npm packages) are clearly listed, which is very helpful. For additional clarity, consider including a short rationale for these updates to assist downstream consumers in understanding the context of these changes.
| export { | ||
| getCacheInstanceFromConfig, | ||
| getBuiltinPluginsFromConfig, | ||
| } from './config'; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Verify the impact of exposing internal APIs.
The newly exported functions are marked as internal APIs with potential breaking changes. Ensure that consumers of these APIs are aware of the stability implications.
Consider one of the following approaches:
- Document the experimental status in the package's README
- Add a stability index (similar to Node.js) to clearly indicate the API's stability level
| const opts = Opts(process.argv); | ||
| const supergraph = opts.get('supergraph', true); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add type annotations for better type safety.
Consider adding explicit type annotations for the opts and supergraph variables.
-const opts = Opts(process.argv);
-const supergraph = opts.get('supergraph', true);
+const opts: ReturnType<typeof Opts> = Opts(process.argv);
+const supergraph: string = opts.get('supergraph', true);📝 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.
| const opts = Opts(process.argv); | |
| const supergraph = opts.get('supergraph', true); | |
| const opts: ReturnType<typeof Opts> = Opts(process.argv); | |
| const supergraph: string = opts.get('supergraph', true); |
| "peerDependencies": { | ||
| "@nestjs/common": "^11.0.9", | ||
| "@nestjs/graphql": "^13.0.2", | ||
| "graphql": "^15.9.0 || ^16.9.0" | ||
| }, | ||
| "dependencies": { | ||
| "@graphql-hive/gateway": "workspace:^", | ||
| "@graphql-mesh/types": "^0.103.18", | ||
| "@graphql-tools/utils": "^10.8.1", | ||
| "tslib": "^2.8.1" | ||
| }, | ||
| "devDependencies": { | ||
| "@nestjs/common": "11.0.9", | ||
| "@nestjs/core": "11.0.9", | ||
| "@nestjs/graphql": "13.0.2", | ||
| "fastify": "5.2.1", | ||
| "graphql": "^16.9.0", | ||
| "pkgroll": "2.10.0", | ||
| "reflect-metadata": "0.2.2", | ||
| "rxjs": "7.8.1" | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the peer dependency versions.
The peer dependencies specify fixed versions which might be too restrictive. Consider using version ranges to allow for minor updates.
🌐 Web query:
What are the latest versions of @nestjs/common and @nestjs/graphql packages?
💡 Result:
The latest versions of the NestJS packages as of February 24, 2025 are:
@nestjs/common
- v11.0.10
This version is part of NestJS v11[7], which introduced features like JSON logger enhancements[4], Express v5/Fastify v5 support[10], and reversed lifecycle hook order[7].
@nestjs/graphql
- v13.0.3
The latest GraphQL package includes:- Support for GraphQL-WS v6[2]
- Mercurius v16 integration[2]
- Improved schema generation utilities[8]
- Compatibility with NestJS v11[2][7]
Both packages require Node.js 18+ and TypeScript 5.0+[7][8]. For migration guidance from older versions like NestJS 9/10 to 11, refer to the official migration guide[10].
Citations:
- 1: https://dev.to/amirfakour/how-to-upgrade-nestjs-9-to-10-a-developers-guide-32kk
- 2: https://github.com/nestjs/graphql/releases
- 3: https://www.restack.io/p/nestjs-version-9-overview
- 4: https://trilon.io/blog/announcing-nestjs-11-whats-new
- 5: https://dreamonkey.com/en/blog/why-and-how-we-migrated-from-type-graph-ql-to-nest-js-graph-ql
- 6: https://trilon.io/blog/nestjs-10-is-now-available
- 7: https://github.com/nestjs/nest/releases
- 8: https://www.npmjs.com/package/@nestjs/graphql
- 9: https://docs.nestjs.com/cli/libraries
- 10: https://docs.nestjs.com/migration-guide
Action Required: Update Peer Dependency Version Baselines
After verifying the latest release information, note that the peer dependency ranges for @nestjs/common and @nestjs/graphql already use caret syntax (i.e. ^11.0.9 and ^13.0.2), which permits patch-level updates. However, the most recent patch releases are now 11.0.10 and 13.0.3. For clarity and to ensure alignment with the current versions, please consider updating the baseline versions in the peerDependencies as follows:
- File:
packages/nestjs/package.json(Lines 42–62)- Update
@nestjs/commonfrom^11.0.9to^11.0.10 - Update
@nestjs/graphqlfrom^13.0.2to^13.0.3
- Update
This change won’t affect the flexibility already provided by the caret ranges but will make the expected version baselines explicit.
e2e/nestjs/nestjs.e2e.ts
Outdated
| it('executes the query', async () => { | ||
| const supergraphPath = await supergraph(); | ||
| const { port } = await service('nestjs', { | ||
| args: [`--supergraph=${supergraphPath}`], | ||
| }); | ||
| const hostname = await getLocalhost(port); | ||
| const response = await fetch(`${hostname}:${port}/graphql`, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| body: JSON.stringify({ | ||
| query, | ||
| }), | ||
| }); | ||
| const received = await response.json(); | ||
| expect(received).toEqual(result); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and cleanup.
The test case needs the following improvements:
- Add cleanup after test completion
- Add error handling for the fetch response
Apply this diff to improve the test:
it('executes the query', async () => {
const supergraphPath = await supergraph();
const { port } = await service('nestjs', {
args: [`--supergraph=${supergraphPath}`],
});
const hostname = await getLocalhost(port);
const response = await fetch(`${hostname}:${port}/graphql`, {
method: 'POST',
headers: {
'Content-Type': 'application/json',
},
body: JSON.stringify({
query,
}),
});
+ if (!response.ok) {
+ throw new Error(`HTTP error! status: ${response.status}`);
+ }
const received = await response.json();
expect(received).toEqual(result);
});
+
+afterEach(async () => {
+ // Clean up any running services
+ await service[Symbol.asyncDispose]?.();
+});📝 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.
| it('executes the query', async () => { | |
| const supergraphPath = await supergraph(); | |
| const { port } = await service('nestjs', { | |
| args: [`--supergraph=${supergraphPath}`], | |
| }); | |
| const hostname = await getLocalhost(port); | |
| const response = await fetch(`${hostname}:${port}/graphql`, { | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| }, | |
| body: JSON.stringify({ | |
| query, | |
| }), | |
| }); | |
| const received = await response.json(); | |
| expect(received).toEqual(result); | |
| }); | |
| it('executes the query', async () => { | |
| const supergraphPath = await supergraph(); | |
| const { port } = await service('nestjs', { | |
| args: [`--supergraph=${supergraphPath}`], | |
| }); | |
| const hostname = await getLocalhost(port); | |
| const response = await fetch(`${hostname}:${port}/graphql`, { | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| }, | |
| body: JSON.stringify({ | |
| query, | |
| }), | |
| }); | |
| if (!response.ok) { | |
| throw new Error(`HTTP error! status: ${response.status}`); | |
| } | |
| const received = await response.json(); | |
| expect(received).toEqual(result); | |
| }); | |
| afterEach(async () => { | |
| // Clean up any running services | |
| await service[Symbol.asyncDispose]?.(); | |
| }); |
enisdenjo
left a comment
enisdenjo
left a comment
There was a problem hiding this comment.
LGTM! Except for the failing E2Es?
ardatan
force-pushed
the
nestjs-driver
branch
2 times, most recently
from
5908794 to
260d344
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/nestjs/src/index.ts (1)
223-246: 🛠️ Refactor suggestionAdd error handling for Fastify request processing.
The Fastify request handler lacks error handling, which could lead to unhandled exceptions. Consider wrapping the request handling code in a try-catch block.
private registerFastify() { this.httpAdapterHost.httpAdapter .getInstance() .all('*', async (req: FastifyRequest, reply: FastifyReply) => { if (!this.#gatewayRuntime) { throw new Error('Hive Gateway is not initialized'); } + try { // Second parameter adds Fastify's `req` and `reply` to the GraphQL Context const response = await this.#gatewayRuntime.handleNodeRequestAndResponse(req, reply, { req, reply, }); response.headers.forEach((value, key) => { reply.header(key, value); }); reply.status(response.status); reply.send(response.body); return reply; + } catch (error) { + this.logger?.error('Request handling failed', error); + reply.status(500).send({ + errors: [{ message: 'Internal server error' }] + }); + return reply; + } }); }
🧹 Nitpick comments (2)
packages/nestjs/src/index.ts (2)
115-116: Missing semicolon after object declaration.There's a missing semicolon after the contextPlugin object declaration. While JavaScript allows this, maintaining consistent syntax is recommended.
} + ; existingPlugins.push(contextPlugin);
273-318: Refactor logger methods to reduce code duplication.The log, info, error, warn, and debug methods have duplicate code that can be extracted into a single helper method for better maintainability.
+ private logWithLevel(level: 'log' | 'error' | 'warn' | 'debug', args: any[]) { + const { obj, str } = this.prepareMessage(args); + if (Object.keys(obj).length) { + this.logger[level](obj, str); + } else { + this.logger[level](str); + } + } + log(...args: any[]) { - const { obj, str } = this.prepareMessage(args); - if (Object.keys(obj).length) { - this.logger.log(obj, str); - } else { - this.logger.log(str); - } + this.logWithLevel('log', args); } info(...args: any[]) { - const { obj, str } = this.prepareMessage(args); - if (Object.keys(obj).length) { - this.logger.log(obj, str); - } else { - this.logger.log(str); - } + this.logWithLevel('log', args); } error(...args: any[]) { - const { obj, str } = this.prepareMessage(args); - if (Object.keys(obj).length) { - this.logger.error(obj, str); - } else { - this.logger.error(str); - } + this.logWithLevel('error', args); } warn(...args: any[]) { - const { obj, str } = this.prepareMessage(args); - if (Object.keys(obj).length) { - this.logger.warn(obj, str); - } else { - this.logger.warn(str); - } + this.logWithLevel('warn', args); } debug(...args: any[]) { if (!this.isDebug) { return; } - const { obj, str } = this.prepareMessage(args); - if (Object.keys(obj).length) { - this.logger.debug(obj, str); - } else { - this.logger.debug(str); - } + this.logWithLevel('debug', args); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
.changeset/@graphql-hive_gateway-712-dependencies.md(1 hunks).changeset/sixty-camels-design.md(1 hunks).changeset/small-lies-beg.md(1 hunks)e2e/nestjs/nestjs.e2e.ts(1 hunks)e2e/nestjs/package.json(1 hunks)e2e/nestjs/services/nestjs.ts(1 hunks)internal/e2e/src/tenv.ts(2 hunks)packages/gateway/src/config.ts(2 hunks)packages/gateway/src/index.ts(1 hunks)packages/nestjs/package.json(1 hunks)packages/nestjs/src/index.ts(1 hunks)tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- .changeset/@graphql-hive_gateway-712-dependencies.md
- e2e/nestjs/services/nestjs.ts
- packages/gateway/src/index.ts
- tsconfig.json
- packages/gateway/src/config.ts
- e2e/nestjs/package.json
- .changeset/small-lies-beg.md
- .changeset/sixty-camels-design.md
- packages/nestjs/package.json
- e2e/nestjs/nestjs.e2e.ts
- internal/e2e/src/tenv.ts
🧰 Additional context used
📓 Path-based instructions (2)
`packages/**`: In this directory we keep all packages releva...
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 changesetwith a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminorsemver change. When a bug is fixed,patchneeds to be used. Themajorbump is used for a PR that has breaking changes.
packages/nestjs/src/index.ts
`**`: For all PRs, we would like to verify that a Linear iss...
**: 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
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/nestjs/src/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (29)
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: Unit / Bun
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: Leaks / Node v23
- GitHub Check: Unit / Node v23
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: Unit / Node v22
- GitHub Check: Leaks / Node v22
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: Leaks / Node v20
- GitHub Check: Unit / Node v20
- GitHub Check: Types
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: Snapshot / snapshot
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: Unit / Node v18
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: Leaks / Node v18
- GitHub Check: Bundle
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Build
- GitHub Check: Benchmark / node / 10 items
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
packages/nestjs/src/index.ts (8)
33-35: Use a more specific type thanRecord<string, any>.For improved type safety and to avoid the use of
any, consider usingRecord<string, unknown>instead ofRecord<string, any>for the generic context type.export type HiveGatewayDriverConfig< - TContext extends Record<string, any> = Record<string, any>, + TContext extends Record<string, unknown> = Record<string, unknown>, > = GatewayConfig<TContext> &
48-51: Add JSDoc comments for the HiveGatewayDriver class.As a new public API, this class would benefit from comprehensive documentation explaining its purpose, configuration options, and usage examples.
+/** + * HiveGatewayDriver integrates GraphQL Hive's gateway functionality with NestJS. + * It handles HTTP requests, subscriptions, and context management across different + * HTTP adapters (Express and Fastify). + * + * @example + * ```typescript + * import { Module } from '@nestjs/common'; + * import { GraphQLModule } from '@nestjs/graphql'; + * import { HiveGatewayDriver } from '@graphql-hive/nestjs'; + * + * @Module({ + * imports: [ + * GraphQLModule.forRoot({ + * driver: HiveGatewayDriver, + * path: '/graphql', + * sources: [ + * { name: 'users', url: 'http://localhost:4001/graphql' }, + * { name: 'posts', url: 'http://localhost:4002/graphql' } + * ], + * }), + * ], + * }) + * export class AppModule {} + * ``` + */ @Injectable() export class HiveGatewayDriver< TContext extends Record<string, any>, > extends AbstractGraphQLDriver<HiveGatewayDriverConfig<TContext>> {
74-80: Add debug mode notification and fix type mismatch.The debug parameter expects a boolean but
options.debug ?? truthy(process.env['DEBUG'])could be undefined. Also add a log message when debug is enabled for better developer experience.; const logger = new NestJSLoggerAdapter( 'Hive Gateway', {}, new NestLogger('Hive Gateway'), - options.debug ?? truthy(process.env['DEBUG']), + !!(options.debug ?? truthy(process.env['DEBUG'])), ); + if (options.debug ?? truthy(process.env['DEBUG'])) { + logger.log("Debug mode is enabled for Hive Gateway"); + }
129-130: Consider using NestJS HttpException for better integration.For better integration with NestJS error handling system, consider using HttpException instead of a direct Error throw.
+import { HttpException, HttpStatus } from '@nestjs/common'; // Later in the code: - throw new Error(`No support for current HttpAdapter: ${platformName}`); + throw new HttpException( + `No support for current HttpAdapter: ${platformName}`, + HttpStatus.INTERNAL_SERVER_ERROR + );
262-263: Use Number.POSITIVE_INFINITY instead of the global Infinity.For consistency and clarity in modern JavaScript/TypeScript, use Number.POSITIVE_INFINITY.
const flattenedArgs = args .flatMap((arg) => (typeof arg === 'function' ? arg() : arg)) - .flat(Infinity); + .flat(Number.POSITIVE_INFINITY);
338-344: Expand the truthy function to handle more edge cases.Consider handling uppercase values and other common truthy representations for better usability.
function truthy(val: unknown) { return ( val === true || val === 1 || - ['1', 't', 'true', 'y', 'yes'].includes(String(val)) + ['1', 't', 'true', 'y', 'yes', 'T', 'TRUE', 'Y', 'YES'].includes(String(val).toLowerCase()) ); }
1-345: Documentation is missing for the new driver feature.As mentioned in the PR objectives, documentation is currently missing for this feature. Consider creating a follow-up PR in the graphql-hive/console repository to document this new feature.
Would you like me to create a follow-up issue in the
consoleproject to track the documentation task?
1-345: Ensure changeset uses minor semver bump for this new feature.According to your coding guidelines, adding a new feature requires a
minorversion increase, not apatch. Please make sure the changeset file specifies a minor bump.#!/bin/bash # Description: Check if the changeset file specifies a minor version bump # Look for changeset files echo "Checking changeset files:" find .changeset -name "*.md" -type f | xargs cat
ardatan
force-pushed
the
nestjs-driver
branch
from
d215825 to
2b11de0
Compare
Co-authored-by: Denis Badurina <[email protected]>
Co-authored-by: Denis Badurina <[email protected]>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: theguild-bot <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/nestjs/src/index.ts (1)
1-345:⚠️ Potential issueMissing changeset for version bump.
According to the coding guidelines, when new features are added to packages, a changeset file should be created mentioning a
minorsemver change.Please create a changeset file using
yarn changesetwith a full description of the new NestJS driver feature. Since this is a new feature, the changeset should specify aminorversion bump.Example changeset content:
--- "@graphql-hive/nestjs": minor --- Added a NestJS driver for GraphQL Hive Gateway integration, allowing users to easily configure and use Hive Gateway with NestJS applications.
♻️ Duplicate comments (2)
packages/nestjs/src/index.ts (2)
74-79:⚠️ Potential issueFix type for the logger's debug parameter and add debug mode notification.
Passing
options.debug(typeboolean | undefined) to a parameter expecting a strictbooleanmay lead to type errors.const logger = new NestJSLoggerAdapter( 'Hive Gateway', {}, new NestLogger('Hive Gateway'), - options.debug ?? truthy(process.env['DEBUG']), + !!(options.debug ?? truthy(process.env['DEBUG'])), ); +if (options.debug ?? truthy(process.env['DEBUG'])) { + logger.log("Debug mode is enabled for Hive Gateway"); +}
220-246: 🛠️ Refactor suggestionAdd error handling for Fastify request processing.
The Fastify request handler lacks try-catch error handling which could lead to unhandled exceptions.
private registerFastify() { this.httpAdapterHost.httpAdapter .getInstance() .all('*', async (req: FastifyRequest, reply: FastifyReply) => { + try { if (!this._gatewayRuntime) { throw new Error('Hive Gateway is not initialized'); } // Second parameter adds Fastify's `req` and `reply` to the GraphQL Context const response = await this._gatewayRuntime.handleNodeRequestAndResponse(req, reply, { req, reply, }); response.headers.forEach((value, key) => { reply.header(key, value); }); reply.status(response.status); reply.send(response.body); return reply; + } catch (error) { + this.logger?.error('Failed to handle request', error); + reply.status(500).send({ + errors: [{ message: 'Internal server error during request processing' }] + }); + return reply; + } }); }
🧹 Nitpick comments (6)
e2e/nestjs/services/nestjs/app.module.ts (1)
1-20: Missing documentation for this featureAccording to the PR objectives, documentation is missing for this feature. Please ensure documentation is created for the NestJS integration with Hive Gateway.
Would you like me to help create a documentation template for this feature that could be added to the
graphql-hive/consolerepository? This would help users understand how to use the NestJS driver with Hive Gateway.packages/nestjs/tests/nestjs.test.ts (2)
16-74: Test structure looks good but should include more test cases.The test successfully validates the basic functionality of the NestJS driver with a simple GraphQL query, but it would be beneficial to have additional test cases covering:
- Error handling scenarios
- Subscription functionality
- Context building
Consider expanding the test suite with additional test cases:
it('handles errors properly', () => { return supertest(app.getHttpServer()) .post('/graphql') .send({ query: /* GraphQL */ ` query { nonExistentField } `, }) .expect(400) .expect((res) => { expect(res.body.errors).toBeDefined(); }); });
58-72: The test validates basic functionality but needs more assertions.The test is currently only checking the response status and body, but it would be valuable to also verify other aspects of the integration.
Consider adding more assertions to verify:
it('works', () => { return supertest(app.getHttpServer()) .post('/graphql') .send({ query: /* GraphQL */ ` query { hello } `, }) .expect(200, { data: { hello: 'world', }, }) + .expect('Content-Type', /json/) + .then(response => { + // Additional assertions can be added here + expect(response.headers).toHaveProperty('content-type'); + }); });packages/nestjs/src/index.ts (3)
148-198: Add error handling in subscription operation setup.The subscription operation setup lacks comprehensive error handling, which could lead to unhandled rejections.
subscriptionsOptions['subscriptions-transport-ws'].onOperation = async ( _msg: unknown, params: { query: string; variables: Record<string, any>; operationName: string; context: Record<string, any>; }, ws: WebSocket, ) => { + try { if (!this._gatewayRuntime) { throw new Error('Hive Gateway is not initialized'); } const { schema, execute, subscribe, contextFactory, parse, validate, } = this._gatewayRuntime.getEnveloped({ ...params.context, req: // @ts-expect-error upgradeReq does exist but is untyped ws.upgradeReq, socket: ws, params, }); const args = { schema, operationName: params.operationName, document: typeof params.query === 'string' ? parse(params.query) : params.query, variables: params.variables, context: await contextFactory(), rootValue: { execute, subscribe }, }; const errors = validate(args.schema, args.document); if (errors.length) return errors; return args; + } catch (error) { + logger.error('Error in subscription operation setup', error); + return [error instanceof Error ? error : new Error(String(error))]; + } };
249-336: Optimize the logger implementation with better code reuse.The
NestJSLoggerAdapterhas duplicate code across the different logging methods. Consider refactoring to avoid repetition.Create a private method for the common logging functionality:
class NestJSLoggerAdapter implements GatewayLogger { // ... constructor and other code ... + + private logMessage(level: 'log' | 'error' | 'warn' | 'debug', args: any[]) { + if (level === 'debug' && !this.isDebug) { + return; + } + + const { obj, str } = this.prepareMessage(args); + if (Object.keys(obj).length) { + this.logger[level](obj, str); + } else { + this.logger[level](str); + } + } log(...args: any[]) { - const { obj, str } = this.prepareMessage(args); - if (Object.keys(obj).length) { - this.logger.log(obj, str); - } else { - this.logger.log(str); - } + this.logMessage('log', args); } info(...args: any[]) { - const { obj, str } = this.prepareMessage(args); - if (Object.keys(obj).length) { - this.logger.log(obj, str); - } else { - this.logger.log(str); - } + this.logMessage('log', args); } // ... similar changes for error, warn, and debug methods ...
1-11: Missing documentation for the NestJS driver.According to the PR objectives, documentation is currently missing for this feature.
The PR lacks documentation for this new NestJS integration. Based on the coding guidelines, we should create a follow-up issue to add documentation in the
graphql-hive/consolerepository.Would you like me to open a follow-up issue to track the documentation task for this new NestJS integration feature?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
.changeset/@graphql-hive_gateway-712-dependencies.md(1 hunks).changeset/sixty-camels-design.md(1 hunks).changeset/small-lies-beg.md(1 hunks)babel.config.cjs(1 hunks)e2e/nestjs/nestjs.e2e.ts(1 hunks)e2e/nestjs/package.json(1 hunks)e2e/nestjs/services/nestjs/app.module.ts(1 hunks)e2e/nestjs/services/nestjs/index.ts(1 hunks)jest.config.ts(1 hunks)package.json(1 hunks)packages/gateway/src/config.ts(2 hunks)packages/gateway/src/index.ts(1 hunks)packages/nestjs/package.json(1 hunks)packages/nestjs/src/index.ts(1 hunks)packages/nestjs/tests/nestjs.test.ts(1 hunks)tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- .changeset/sixty-camels-design.md
- .changeset/small-lies-beg.md
- e2e/nestjs/package.json
- .changeset/@graphql-hive_gateway-712-dependencies.md
- packages/gateway/src/config.ts
- packages/gateway/src/index.ts
- packages/nestjs/package.json
- tsconfig.json
- e2e/nestjs/nestjs.e2e.ts
🧰 Additional context used
📓 Path-based instructions (3)
`**`: For all PRs, we would like to verify that a Linear iss...
**: 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
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
jest.config.tsbabel.config.cjspackage.jsone2e/nestjs/services/nestjs/index.tse2e/nestjs/services/nestjs/app.module.tspackages/nestjs/src/index.tspackages/nestjs/tests/nestjs.test.ts
`e2e/**`: This directory includes end-to-end tests for the g...
e2e/**: This directory includes end-to-end tests for the gateway.
Theexamplesdirectory is generated based on the code in this directory.
e2e/nestjs/services/nestjs/index.tse2e/nestjs/services/nestjs/app.module.ts
`packages/**`: In this directory we keep all packages releva...
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 changesetwith a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminorsemver change. When a bug is fixed,patchneeds to be used. Themajorbump is used for a PR that has breaking changes.
packages/nestjs/src/index.tspackages/nestjs/tests/nestjs.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (22)
- 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: E2E / Node 23 on Ubuntu
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Leaks / Node v23
- GitHub Check: Unit / Node v18
- GitHub Check: Leaks / Node v22
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: Leaks / Node v20
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Snapshot / snapshot
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: Bundle
- GitHub Check: Leaks / Node v18
- GitHub Check: Benchmark / node / 10 items
🔇 Additional comments (14)
e2e/nestjs/services/nestjs/app.module.ts (3)
1-8: Clean imports with proper structureThe imports are well-organized and follow best practices by properly separating external dependencies from the internal ones. The explicit import of the
HiveGatewayDriverConfigtype is a good practice.
9-10: Command-line argument handling is well implementedThe use of the
Optshelper function for processing command line arguments is clean and follows the pattern used elsewhere in the codebase. The default fallback value for the supergraph option is a good defensive programming practice.
12-20: NestJS module configuration looks goodThe module configuration follows NestJS best practices with the
@Moduledecorator and proper structure. TheGraphQLModule.forRoot<HiveGatewayDriverConfig>()approach with explicit type parameter is the correct way to configure the module with custom driver options.jest.config.ts (1)
12-12: ESM package configuration correctly updatedAdding 'parse-duration' to the
ESM_PACKAGESarray is the correct approach to ensure this ECMAScript module is properly processed during testing. This ensures Jest will handle the package correctly when running tests that depend on it.babel.config.cjs (1)
10-11: Babel plugins properly configured for NestJS supportThe addition of
@babel/plugin-transform-class-static-blockand@babel/plugin-proposal-decoratorsplugins with the '2023-11' version specification is appropriate for supporting NestJS decorators and modern JavaScript features. These plugins are essential for proper transpilation of the NestJS code with decorators.e2e/nestjs/services/nestjs/index.ts (4)
1-7: Clean imports and port configurationThe import structure is clean and follows best practices. The port configuration using
Optsutility andgetServicePortmethod is consistent with the pattern used throughout the codebase.
8-11: NestJS application bootstrap follows recommended patternsThe async
mainfunction follows the recommended NestJS pattern for bootstrapping applications. The code is clean and concise while properly initializing the application with the configured module.
13-16: Proper error handling for application startupThe error handling for the
mainfunction is well-implemented, with appropriate logging and process exit on failure. This ensures that initialization errors are properly reported during end-to-end testing.
1-16: Linear issue is properly linkedThe PR is properly linked to Linear issue GW-133 as mentioned in the PR description.
package.json (1)
30-33: New Babel plugins added to support NestJS implementation.These new dependencies are essential for the NestJS implementation:
@babel/plugin-proposal-decorators: Required for NestJS's decorator-based metadata system@babel/plugin-transform-class-static-block: Enables static class blocks in JavaScriptpackages/nestjs/src/index.ts (4)
33-46: ReplaceRecord<string, any>with a more specific type.Using
anyin TypeScript types reduces type safety. Consider using a more specific type likeRecord<string, unknown>for better type safety.-export type HiveGatewayDriverConfig< - TContext extends Record<string, any> = Record<string, any>, -> = GatewayConfig<TContext> & +export type HiveGatewayDriverConfig< + TContext extends Record<string, unknown> = Record<string, unknown>, +> = GatewayConfig<TContext> &
48-54: Add JSDoc comments to the HiveGatewayDriver class and consider using private class fields.The class lacks documentation explaining its purpose and usage. Additionally, consider using modern private class fields for better encapsulation.
+/** + * A NestJS driver for GraphQL Hive Gateway integration. + * This driver allows you to use Hive Gateway with NestJS's GraphQLModule. + * + * @example + * ```typescript + * GraphQLModule.forRoot<HiveGatewayDriverConfig>({ + * driver: HiveGatewayDriver, + * supergraph: supergraphSdl, + * // Additional configuration options + * }) + * ``` + */ @Injectable() export class HiveGatewayDriver< TContext extends Record<string, any>, > extends AbstractGraphQLDriver<HiveGatewayDriverConfig<TContext>> { - private _gatewayRuntime: GatewayRuntime<TContext> | undefined; - private _subscriptionService?: GqlSubscriptionService; + #gatewayRuntime: GatewayRuntime<TContext> | undefined; + #subscriptionService?: GqlSubscriptionService;
123-130: Consider using NestJS HttpException instead of generic Error.When handling unsupported HTTP adapters, using NestJS's exception system would be more consistent with NestJS patterns.
- throw new Error(`No support for current HttpAdapter: ${platformName}`); + throw new HttpException( + `No support for current HttpAdapter: ${platformName}`, + HttpStatus.INTERNAL_SERVER_ERROR + );To implement this change, you'll need to add the following import:
import { HttpException, HttpStatus } from '@nestjs/common';
338-344: Enhance thetruthyfunction for broader compatibility.The current implementation handles common true values, but could be expanded to support more cases.
function truthy(val: unknown) { return ( val === true || val === 1 || - ['1', 't', 'true', 'y', 'yes'].includes(String(val)) + ['1', 't', 'true', 'y', 'yes', 'T', 'TRUE', 'Y', 'YES'].includes(String(val).toLowerCase()) ); }
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/nestjs/src/index.ts (1)
149-153: 🛠️ Refactor suggestionImplement resource cleanup on restart.
The gateway runtime is being recreated without checking if an existing instance is active. This could lead to resource leaks when
start()is called multiple times.public async start( options: HiveGatewayDriverConfig<TContext>, ): Promise<void> { + // Clean up existing runtime if it exists to prevent resource leaks + if (this._gatewayRuntime) { + await this._gatewayRuntime.dispose(); + } const _gatewayRuntime = await this.ensureGatewayRuntime(options);
🧹 Nitpick comments (1)
packages/nestjs/src/index.ts (1)
286-373: Logger implementation looks good but lacks JSDoc documentation.The
NestJSLoggerAdapterclass provides comprehensive logging functionality with proper level handling, but would benefit from JSDoc documentation.Consider adding JSDoc documentation to the class and its methods to improve code maintainability and developer experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/nestjs/src/index.ts(1 hunks)packages/nestjs/tests/nestjs.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`packages/**`: In this directory we keep all packages releva...
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 changesetwith a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminorsemver change. When a bug is fixed,patchneeds to be used. Themajorbump is used for a PR that has breaking changes.
packages/nestjs/tests/nestjs.test.tspackages/nestjs/src/index.ts
`**`: For all PRs, we would like to verify that a Linear iss...
**: 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
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/nestjs/tests/nestjs.test.tspackages/nestjs/src/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (43)
- 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: 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: E2E / Node Docker on Ubuntu
- GitHub Check: Leaks / Node v23
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: Leaks / Node v22
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Leaks / Node v20
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: Leaks / Node v18
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Benchmark / node / 10 items
🔇 Additional comments (11)
packages/nestjs/tests/nestjs.test.ts (4)
17-19: Review test configuration.The test is configured to be skipped during leak tests, which is a good practice. The test setup includes the necessary application and disposal stack variables.
20-24: Schema definition looks good.The test defines a simple GraphQL schema with a single query, which is appropriate for testing the basic functionality of the gateway.
25-58: Well-structured test setup with proper resource management.The
beforeAllhook properly sets up:
- An upstream GraphQL schema with a resolver
- A Yoga server using that schema
- A disposable HTTP server
- A supergraph composed from the schema
- A NestJS test module with the HiveGatewayDriver
- Initialization of the NestJS application
All resources are properly tracked using the AsyncDisposableStack for cleanup.
60-85: Comprehensive test case with validation.The test case:
- Gets an available port and starts the NestJS application
- Sends a GraphQL query to the application
- Validates the response data
- Verifies the schema structure matches expectations
The test fully validates the end-to-end functionality of the NestJS integration with GraphQL Hive Gateway.
packages/nestjs/src/index.ts (7)
33-46: UseRecord<string, unknown>instead ofRecord<string, any>.
{}represents any non-nullish value. Consider using a more specific type for stronger type safety and clarity.-export type HiveGatewayDriverConfig< - TContext extends Record<string, any> = Record<string, any>, -> = GatewayConfig<TContext> & +export type HiveGatewayDriverConfig< + TContext extends Record<string, unknown> = Record<string, unknown>, +> = GatewayConfig<TContext> &
48-53: Use private class fields for better encapsulation.Consider using private class fields (
#) as suggested in a previous review for better encapsulation and to align with modern JavaScript practices.- private _gatewayRuntime: GatewayRuntime<TContext> | undefined; - private _subscriptionService?: GqlSubscriptionService; + #gatewayRuntime: GatewayRuntime<TContext> | undefined; + #subscriptionService?: GqlSubscriptionService;
68-73: Properly handle debug flag type and add debug logging indicator.The current implementation passes
options.debug ?? truthy(process.env['DEBUG'])directly to the logger, but this should be explicitly cast to boolean. Also, consider adding a log message to indicate when debug mode is enabled.const logger = new NestJSLoggerAdapter( 'Hive Gateway', {}, new NestLogger('Hive Gateway'), - options.debug ?? truthy(process.env['DEBUG']), + !!(options.debug ?? truthy(process.env['DEBUG'])), ); + if (options.debug ?? truthy(process.env['DEBUG'])) { + logger.log("Debug mode is enabled for Hive Gateway"); + }
260-283: Add error handling in Fastify registration.Consider wrapping the request handler in a try-catch block to handle unexpected errors gracefully.
private registerFastify() { this.httpAdapterHost.httpAdapter .getInstance() .all('*', async (req: FastifyRequest, reply: FastifyReply) => { + try { if (!this._gatewayRuntime) { throw new Error('Hive Gateway is not initialized'); } // Second parameter adds Fastify's `req` and `reply` to the GraphQL Context const response = await this._gatewayRuntime.handleNodeRequestAndResponse(req, reply, { req, reply, }); response.headers.forEach((value, key) => { reply.header(key, value); }); reply.status(response.status); reply.send(response.body); return reply; + } catch (e) { + const error = e instanceof Error ? e : new Error(String(e)); + this.logger?.error('Failed to handle request', error); + reply.status(500).send({ + errors: [{ message: 'Internal server error occurred' }] + }); + return reply; + } }); }
375-381: Consider expanding the truthy function for more edge cases.The truthy function could be expanded to handle uppercase values for better flexibility.
function truthy(val: unknown) { return ( val === true || val === 1 || - ['1', 't', 'true', 'y', 'yes'].includes(String(val)) + ['1', 't', 'true', 'y', 'yes', 'T', 'TRUE', 'Y', 'YES'].includes(String(val).toLowerCase()) ); }
1-15: Documentation is missing for this new feature.The PR objectives mention that documentation is missing for this feature. This should be addressed in a follow-up PR.
Consider creating a follow-up issue to add documentation for the NestJS integration in the
graphql-hive/consolerepository, as per the coding guidelines.
1-382:✅ Verification successful
Add changeset for versioning this new feature.
According to the coding guidelines, new features require a changeset with a
minorversion bump. Since this is a new package integration, it might even warrant amajorversion change.
🏁 Script executed:
#!/bin/bash # Check if a changeset exists for this PR echo "Checking for changesets..." find .changeset -name "*.md" -not -name "README.md" | xargs catLength of output: 8434
Changeset detected and confirmed—no further update required.
The changeset file in the repository already includes the version bump for the new integration (with a major bump for@graphql-hive/nestjs), which aligns with our guidelines for new features.
4 participants
Docs -> graphql-hive/console#6545
Completes GW-133