fix: link merging #6964
fix: link merging #6964
Conversation
|
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
WalkthroughAdded link-directive helpers and updated directive-merge logic so @link directives are compared by their Changes
Sequence Diagram(s)sequenceDiagram
participant Merger as mergeDirectives
participant Match as isMatchingDirective
participant Distinct as isDirectiveDistinctByArguments
participant LinkUtil as isLinkDirective / getLinkDirectiveURL
Merger->>Match: compare incoming directive with existing
Match->>Distinct: determine distinctness for @link
Distinct->>LinkUtil: check if both are @link and fetch URLs
LinkUtil-->>Distinct: return URLs (or undefined)
Distinct-->>Match: return distinctness result
Match-->>Merger: indicate merge or push-as-new
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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 |
| import { isSome } from '@graphql-tools/utils'; | ||
| import { Config } from './merge-typedefs.js'; | ||
|
|
||
| const isLinkDirective = (directive: DirectiveNode): boolean => directive.name.value === 'link'; |
There was a problem hiding this comment.
not sure if we already have AST narrowing fns. Further, I didn't see a LinkDirective type, which woulda been nice. Not going to add anything like that without discussion :)
![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 (2)
packages/merge/src/typedefs-mergers/directives.ts (2)
13-18: Consider handling non-string URL arguments
Currently, the function only checks for aStringValue. If there's a scenario whereurlcould be of a different type, the function might silently ignore it.
20-29: Potential extension to handle arguments for other directives
Right now, non-@linkdirectives are never considered distinct by their arguments. If future requirements demand argument-based distinctness checks across all directives, consider generalizing this logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/merge/src/typedefs-mergers/directives.ts(2 hunks)packages/merge/tests/merge-typedefs.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- 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
🔇 Additional comments (5)
packages/merge/src/typedefs-mergers/directives.ts (4)
11-12: Straightforward name check for@linkdirective
Implementation is concise and does what it says—no issues identified.
30-30: Simple, readable matching predicate
This helper improves clarity by centralizing the directive name comparison.
36-39: Consistent logic for directive existence check
By deferring arguments-based distinctness toisDirectiveDistinctByArguments, this function cleanly separates concerns.
125-125: Confirm handling of multiple matching directives
findIndexreturns the first matching directive. If the schema contains multiple directives with the same name, confirm that this logic is appropriate.packages/merge/tests/merge-typedefs.spec.ts (1)
1707-1738: Excellent validation of@linkdirective merging
This test thoroughly ensures that identical directives are merged while distinct ones (i.e., those with different URLs) are preserved.
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
ardatan
force-pushed
the
fix/link-merging
branch
from
dcefcd5 to
c155133
Compare
|
Thanks for the PR! Sorry for the late review! |
🚨 IMPORTANT: Please do not create a Pull Request without creating an issue first.
Any change needs to be discussed before proceeding. Failure to do so may result in the rejection of
the pull request.
Description
Link directives are not correctly merged correctly when the URL matches.
Closes #6929 (comment)
Type of change
Please delete options that are not relevant.
expected)
Screenshots/Sandbox (if appropriate/relevant):
Adding links to sandbox or providing screenshots can help us understand more about this PR and take
action on it as appropriate
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can
reproduce. Please also list any relevant details for your test configuration
Test Environment:
@graphql-tools/...:Checklist:
CONTRIBUTING doc and the
style guidelines of this project
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose
the solution you did and what alternatives you considered, etc...