-
-
Notifications
You must be signed in to change notification settings - Fork 831
fix(mergeDeep): handle undefined
sources correctly
#7012
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: 28b79a3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 26 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Warning Rate limit exceeded@ardatan has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 47 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 (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis pull request introduces a patch for the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Caller
participant M as mergeDeep Function
participant S as Source Item
C->>M: Call mergeDeep(sources)
alt sources array is empty
M-->>C: Return undefined
else sources length is one
M-->>C: Return the single source
else
loop For each source in sources
alt Source is nullish
M->>M: Skip this source
else
M->>M: Merge source into result
end
end
M-->>C: Return merged result
end
Poem
✨ Finishing touches
🧪 Generate unit tests
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. Comment |
🚀 Snapshot Release (
|
Package | Version | Info |
---|---|---|
@graphql-tools/executor |
1.4.5-alpha-20250307121300-14365b34ed1b50dd48041446ce0a72986ae45a1c |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-apollo-link |
1.0.18-alpha-20250307121300-14365b34ed1b50dd48041446ce0a72986ae45a1c |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-envelop |
3.0.26-alpha-20250307121300-14365b34ed1b50dd48041446ce0a72986ae45a1c |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-legacy-ws |
1.1.16-alpha-20250307121300-14365b34ed1b50dd48041446ce0a72986ae45a1c |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-urql-exchange |
1.0.18-alpha-20250307121300-14365b34ed1b50dd48041446ce0a72986ae45a1c |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-yoga |
3.0.26-alpha-20250307121300-14365b34ed1b50dd48041446ce0a72986ae45a1c |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/graphql-tag-pluck |
8.3.18-alpha-20250307121300-14365b34ed1b50dd48041446ce0a72986ae45a1c |
npm ↗︎ unpkg ↗︎ |
graphql-tools |
9.0.17-alpha-20250307121300-14365b34ed1b50dd48041446ce0a72986ae45a1c |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/import |
7.0.17-alpha-20250307121300-14365b34ed1b50dd48041446ce0a72986ae45a1c |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/links |
9.0.26-alpha-20250307121300-14365b34ed1b50dd48041446ce0a72986ae45a1c |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/load |
8.0.18-alpha-20250307121300-14365b34ed1b50dd48041446ce0a72986ae45a1c |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/apollo-engine-loader |
8.0.19-alpha-20250307121300-14365b34ed1b50dd48041446ce0a72986ae45a1c |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/code-file-loader |
8.1.19-alpha-20250307121300-14365b34ed1b50dd48041446ce0a72986ae45a1c |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/git-loader |
8.0.23-alpha-20250307121300-14365b34ed1b50dd48041446ce0a72986ae45a1c |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/github-loader |
8.0.19-alpha-20250307121300-14365b34ed1b50dd48041446ce0a72986ae45a1c |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/graphql-file-loader |
8.0.18-alpha-20250307121300-14365b34ed1b50dd48041446ce0a72986ae45a1c |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/json-file-loader |
8.0.17-alpha-20250307121300-14365b34ed1b50dd48041446ce0a72986ae45a1c |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/module-loader |
8.0.17-alpha-20250307121300-14365b34ed1b50dd48041446ce0a72986ae45a1c |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/url-loader |
8.0.30-alpha-20250307121300-14365b34ed1b50dd48041446ce0a72986ae45a1c |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/merge |
9.0.23-alpha-20250307121300-14365b34ed1b50dd48041446ce0a72986ae45a1c |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/mock |
9.0.21-alpha-20250307121300-14365b34ed1b50dd48041446ce0a72986ae45a1c |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/node-require |
7.0.19-alpha-20250307121300-14365b34ed1b50dd48041446ce0a72986ae45a1c |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/relay-operation-optimizer |
7.0.18-alpha-20250307121300-14365b34ed1b50dd48041446ce0a72986ae45a1c |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/resolvers-composition |
7.0.17-alpha-20250307121300-14365b34ed1b50dd48041446ce0a72986ae45a1c |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/schema |
10.0.22-alpha-20250307121300-14365b34ed1b50dd48041446ce0a72986ae45a1c |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/utils |
10.8.5-alpha-20250307121300-14365b34ed1b50dd48041446ce0a72986ae45a1c |
npm ↗︎ unpkg ↗︎ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/utils/tests/mergeDeep.test.ts (1)
76-86
: Consider adding a test for array with only undefined valuesThe current tests cover empty arrays and arrays with some undefined values, but there's no explicit test for
mergeDeep([undefined])
which is mentioned in the changeset. Adding such a test would ensure complete coverage of the scenarios described in the changeset.it('returns undefined when an empty sources array passed', () => { expect(mergeDeep([])).toEqual(undefined); }); + + it('returns undefined when array contains only undefined values', () => { + expect(mergeDeep([undefined])).toEqual(undefined); + expect(mergeDeep([undefined, null, undefined])).toEqual(undefined); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/chilly-icons-guess.md
(1 hunks)packages/utils/src/mergeDeep.ts
(2 hunks)packages/utils/tests/mergeDeep.test.ts
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Unit Test on Node 18 (windows-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 23 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 23 (ubuntu-latest) and GraphQL v15
- GitHub Check: Unit Test on Node 22 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 22 (ubuntu-latest) and GraphQL v15
- GitHub Check: Unit Test on Node 20 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 20 (ubuntu-latest) and GraphQL v15
- GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v15
- GitHub Check: Unit Test on Bun (ubuntu-latest) and GraphQL v16
- GitHub Check: Full Check on GraphQL v16
- GitHub Check: Unit Test on Bun (ubuntu-latest) and GraphQL v15
- GitHub Check: deployment
🔇 Additional comments (6)
.changeset/chilly-icons-guess.md (1)
1-11
: Well-documented changeset for the bug fixThe changeset clearly describes the bug fix in the
mergeDeep
function and provides specific examples of inputs and their expected outputs. This documentation helps users understand the new behavior and is aligned with the implementation changes.packages/utils/src/mergeDeep.ts (2)
15-20
: LGTM: Early return for edge cases improves function clarityThese additions handle the edge cases as described in the changeset:
- Returning
undefined
when an empty array is passed- Returning the single source when only one item is provided
This is a good optimization that avoids unnecessary processing when the result is deterministic.
63-65
: LGTM: Properly skipping null/undefined valuesThis change correctly implements the behavior described in the changeset, where nullish values in the sources array are ignored during the merge process. This prevents errors that could occur when trying to process
null
orundefined
as objects.packages/utils/tests/mergeDeep.test.ts (3)
76-78
: Good test for filtering nullish valuesThis test case properly verifies that the function ignores
undefined
values in the sources array, aligning with the fix implemented in the main function.
80-82
: Good test for single empty objectThis test confirms that the function correctly handles a source array containing only an empty object, returning that empty object as expected.
84-86
: Good test for empty sources arrayThis test verifies the new behavior where an empty sources array returns
undefined
, which is consistent with the function implementation.
💻 Website PreviewThe latest changes are available as preview in: https://pr-7012.graphql-tools.pages.dev |
14365b3
to
8f39b80
Compare
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
3751f85
to
1622faf
Compare
Fixes #7011
The following inputs and outputs are corrected;
mergeDeep([{a:2} undefined])
- Any nullish values should be ignored so it should return{a:2}
mergeDeep([])
- no sources should returnundefined
mergeDeep([undefined])
- no sources should returnundefined