fix(utils): use specifiedDirectives directly to check if it is a native directive
#7716
Conversation
🦋 Changeset detectedLatest commit: c52cd46 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 |
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThis PR replaces the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
💻 Website PreviewThe latest changes are available as preview in: https://pr-7716.graphql-tools.pages.dev |
![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/schema/tests/reproductions.test.ts (1)
5-27: Good test coverage for the primary use case.The test correctly validates that a custom
@deprecateddirective with a different signature is preserved when printing the schema, which is the core issue this PR addresses.Consider adding test cases for:
- Verifying the custom
@deprecateddirective actually applies its custom fields (not just that it prints correctly)- Testing with other native directives like
@specifiedBy,@skip,@include- Validating the rewiring behavior specifically (not just schema printing)
Example additional test:
test('custom directive with native name is rewired correctly', () => { const typeDefs = /* GraphQL */ ` directive @skip(if: Boolean!, metadata: String!) on FIELD type Query { field: String @skip(if: true, metadata: "test") } `; const schema = makeExecutableSchema({ typeDefs }); const queryType = schema.getType('Query'); // Verify custom @skip with metadata field exists });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/jolly-drinks-sip.md(1 hunks)packages/schema/tests/reproductions.test.ts(1 hunks)packages/utils/src/mapSchema.ts(0 hunks)packages/utils/src/print-schema-with-directives.ts(1 hunks)packages/utils/src/rewire.ts(2 hunks)
💤 Files with no reviewable changes (1)
- packages/utils/src/mapSchema.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-29T19:58:05.749Z
Learnt from: ardatan
Repo: ardatan/graphql-tools PR: 0
File: :0-0
Timestamp: 2025-01-29T19:58:05.749Z
Learning: The `isUrl` helper in graphql-tools/utils should be tested with both URL.canParse and new URL() paths, covering various URL formats (http, https, file) and invalid cases. Tests should properly mock and restore URL.canParse to ensure consistent behavior across environments.
Applied to files:
packages/schema/tests/reproductions.test.ts
🧬 Code graph analysis (1)
packages/schema/tests/reproductions.test.ts (2)
packages/schema/src/makeExecutableSchema.ts (1)
makeExecutableSchema(51-107)packages/utils/src/print-schema-with-directives.ts (1)
printSchemaWithDirectives(118-124)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Unit Test on Node 18 (windows-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v15
- GitHub Check: Unit Test on Node 24 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Bun
- GitHub Check: Unit Test on Node 22 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v16
- GitHub Check: Full Check on GraphQL v16
- GitHub Check: deployment
🔇 Additional comments (5)
packages/utils/src/rewire.ts (2)
26-26: LGTM: Import updated correctly.The import change from
isSpecifiedDirectivetospecifiedDirectivesis appropriate for the new reference-based equality check.
77-79: Approve: Reference equality correctly prevents rewiring native directives.The change from name-based comparison to reference equality (
===) is complete and correctly implemented. Verification confirms:
isSpecifiedDirectivefunction has been fully removed from the codebase- New reference-based approach using
specifiedDirectives.some(specifiedDirective => specifiedDirective === directive)is properly applied in bothrewire.ts(line 77) andprint-schema-with-directives.ts(line 77)- This correctly distinguishes between native directive instances (which maintain reference equality with GraphQL's
specifiedDirectives) and custom directives with native names (which are different instances and can be rewired).changeset/jolly-drinks-sip.md (1)
1-20: LGTM: Changeset clearly documents the fix.The changeset accurately explains the problem (name-based comparison allowing native directive overrides) and the solution (reference equality check).
packages/utils/src/print-schema-with-directives.ts (2)
77-79: Approve: Reference equality for directive definitions.The change correctly uses reference equality to determine whether to skip printing a directive definition. This ensures custom directives with native names (like a custom
@deprecated) have their definitions included in the output.
240-244: The review comment is based on incorrect code analysis and should be disregarded.The original analysis misunderstands the code flow. At line 240-244, the code doesn't filter out custom directives—it separates them from native ones. The custom directives are preserved in
directiveNodesBesidesNativeDirectives. At line 245, the code searches for native@deprecatedin the original directives array and only reconstructs it if missing (line 250). Custom@deprecateddirectives in extensions would not be filtered out; they'd be found and preserved.However, there is a related but distinct issue: the code assumes any directive with
name.value === 'deprecated'is the GraphQL-specified native directive, which could misidentify custom directives with that name. But this is a different concern than what the review claims.Likely an incorrect or invalid review comment.
Workaround for a bug in the upstream graphql-js graphql/graphql-js#4511
Use
specifiedDirectives.someinstead ofisSpecifiedDirectiveto check if the directive is anative directive
In case of overriding a native directive like
@deprecated,isSpecifiedDirectiveonly checks the name like;But we need to check the actual reference equality to avoid rewiring native directives like below;