Add Node 16 to promise-helpers to prevent breaking change on dependent Tools packages #2123
Conversation
|
Warning Rate limit exceeded@ardatan has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 5 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughA new changeset entry has been added to document a patch for the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
🚀 Snapshot Release (
|
| Package | Version | Info |
|---|---|---|
@whatwg-node/promise-helpers |
1.2.2-alpha-20250303010559-a8c1affa732f19dd296462b66215c01fd144d661 |
npm ↗︎ unpkg ↗︎ |
@whatwg-node/server |
0.9.71-alpha-20250303010559-a8c1affa732f19dd296462b66215c01fd144d661 |
npm ↗︎ unpkg ↗︎ |
✅
|
✅
|
✅
|
✅
|
✅
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/server/test/test-server.ts (1)
170-170: Consider replacing empty object type.The empty object type
{}should be replaced with a more explicit type definition.- ServerAdapterBaseObject<{}> + ServerAdapterBaseObject<Record<string, unknown>>🧰 Tools
🪛 Biome (1.9.4)
[error] 170-170: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/fetch/dist/create-node-ponyfill.jsis excluded by!**/dist/**
📒 Files selected for processing (2)
packages/server/test/test-fetch.ts(1 hunks)packages/server/test/test-server.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/server/test/test-server.ts
[error] 170-170: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: unit / node 23
- GitHub Check: unit / node 22
- GitHub Check: unit / node 20
- GitHub Check: unit / node 18
- GitHub Check: e2e / cloudflare-workers
- GitHub Check: unit / node 16
- GitHub Check: e2e / azure-function
- GitHub Check: e2e / aws-lambda
🔇 Additional comments (5)
packages/server/test/test-fetch.ts (1)
82-82: Improved conditional test execution for Deno environment.The updated condition ensures that the 'Native' test suite will only run in Deno environments where the fetch API is actually available, preventing potential test failures in environments where
fetchmight not be accessible.packages/server/test/test-server.ts (4)
5-5: LGTM: Improved import statement.Changed from importing Fastify to only importing Fastify types, which is more efficient and aligns with the dynamic import pattern implemented below.
12-13: Good approach for version detection.Extracting the Node.js major version into a variable makes the conditional logic more readable and maintainable.
162-246: Effective Node.js version compatibility implementation.Conditionally including the Fastify implementation only for Node.js 16+ is a good approach to maintaining backward compatibility while preventing breaking changes.
🧰 Tools
🪛 Biome (1.9.4)
[error] 170-170: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
172-172: Well-implemented dynamic import pattern.Using dynamic import with fallback to handle both default and named exports accommodates different module formats across Node.js versions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/server/test/test-server.ts (1)
162-486: Well-structured conditional framework support based on Node version.Wrapping the Fastify, Express, Koa, and Hapi server implementations in a Node version check ensures the tests only run on compatible environments. The dynamic import of Fastify using
await import('fastify').then(m => m.default || m)properly handles both ESM and CommonJS module formats.There are three instances of using
{}as a type which static analysis flagged. While not directly related to Node 16 compatibility, consider using more specific types:- ServerAdapterBaseObject<{}> + ServerAdapterBaseObject<Record<string, unknown>>🧰 Tools
🪛 Biome (1.9.4)
[error] 170-170: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 301-301: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 394-394: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/server/src/utils.ts(1 hunks)packages/server/test/node.spec.ts(1 hunks)packages/server/test/test-server.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/server/test/test-server.ts
[error] 170-170: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 301-301: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 394-394: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: unit / node 23
- GitHub Check: unit / node 22
- GitHub Check: unit / deno
- GitHub Check: unit / node 20
- GitHub Check: unit / node 18
- GitHub Check: unit / bun
- GitHub Check: server (undici)
- GitHub Check: unit / node 16
- GitHub Check: e2e / cloudflare-modules
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: type check
- GitHub Check: alpha / snapshot
- GitHub Check: server (ponyfill)
- GitHub Check: prettier
- GitHub Check: e2e / cloudflare-workers
- GitHub Check: lint
- GitHub Check: e2e / azure-function
- GitHub Check: node-fetch (noConsumeBody)
- GitHub Check: e2e / aws-lambda
- GitHub Check: server (native)
🔇 Additional comments (4)
packages/server/src/utils.ts (1)
259-265: Great addition of server response finished check.This adds a necessary check to prevent calling
end()on a server response that is already finished, which is essential for compatibility with Node 16 and prevents potential errors.packages/server/test/node.spec.ts (1)
533-534: Well implemented conditional test execution pattern.This is a good approach to conditionally run tests based on feature availability. The
testIfhelper ensures the React streaming response test is skipped in environments like Node 16 whereReadableStreammight not be globally available by default.packages/server/test/test-server.ts (2)
12-13: Appropriate Node.js version detection.Extracting the major version number is a clean approach to conditionally execute code based on Node version compatibility.
5-5: Good change to import only types from Fastify.Changing from importing the entire module to only importing types is a better practice for type checking without loading the module unnecessarily.
Fixes ardatan/graphql-tools#6983
Summary by CodeRabbit
Chores
Tests
Node Specific Casestest suite to conditionally execute tests based on the availability ofReadableStream.