-
-
Notifications
You must be signed in to change notification settings - Fork 834
Support 'extend schema' syntax and add 'link' to the list of builtins #7225
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: 94276ef The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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
Summary by CodeRabbit
WalkthroughSupport for the 'extend schema' syntax and the 'link' directive was added to the GraphQL import package. The import logic was updated to recognize schema extensions, process their directives, and handle the 'link' directive as a built-in. New tests and fixtures verify correct import and traversal of these schema features. Changes
Sequence Diagram(s)sequenceDiagram
participant Importer as importSchema()
participant File as h.graphql
participant Parser as parse()
participant Visitor as visitDefinition()
Importer->>File: Load schema SDL
Importer->>Parser: Parse SDL
Parser->>Visitor: Traverse definitions
Visitor->>Visitor: Handle SchemaExtensionNode
Visitor->>Visitor: Process @link directive
Visitor->>Visitor: Import custom directive (@foo)
Visitor->>Visitor: Process extended types and fields
Visitor->>Importer: Return imported schema
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/import/src/index.tsOops! Something went wrong! :( ESLint: 9.27.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
💻 Website PreviewThe latest changes are available as preview in: https://pr-7225.graphql-tools.pages.dev |
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/quick-chairs-cover.md(1 hunks)packages/import/src/index.ts(5 hunks)packages/import/tests/schema/fixtures/directive/h.graphql(1 hunks)packages/import/tests/schema/import-schema.spec.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v15
packages/import/src/index.ts
[failure] 275-275:
Argument of type '"OperationDefinition" | "SchemaDefinition" | "SchemaExtension"' is not assignable to parameter of type '"SchemaDefinition" | "SchemaExtension"'.
🪛 GitHub Check: Unit Test on Node 20 (ubuntu-latest) and GraphQL v15
packages/import/src/index.ts
[failure] 275-275:
Argument of type '"OperationDefinition" | "SchemaDefinition" | "SchemaExtension"' is not assignable to parameter of type '"SchemaDefinition" | "SchemaExtension"'.
🪛 GitHub Check: Unit Test on Node 23 (ubuntu-latest) and GraphQL v15
packages/import/src/index.ts
[failure] 275-275:
Argument of type '"OperationDefinition" | "SchemaDefinition" | "SchemaExtension"' is not assignable to parameter of type '"SchemaDefinition" | "SchemaExtension"'.
🪛 GitHub Check: Unit Test on Bun (ubuntu-latest) and GraphQL v15
packages/import/src/index.ts
[failure] 275-275:
Argument of type '"OperationDefinition" | "SchemaDefinition" | "SchemaExtension"' is not assignable to parameter of type '"SchemaDefinition" | "SchemaExtension"'.
🪛 GitHub Check: Unit Test on Node 22 (ubuntu-latest) and GraphQL v15
packages/import/src/index.ts
[failure] 275-275:
Argument of type '"OperationDefinition" | "SchemaDefinition" | "SchemaExtension"' is not assignable to parameter of type '"SchemaDefinition" | "SchemaExtension"'.
🪛 GitHub Check: Type Check on GraphQL v15
packages/import/src/index.ts
[failure] 275-275:
Argument of type '"OperationDefinition" | "SchemaDefinition" | "SchemaExtension"' is not assignable to parameter of type '"SchemaDefinition" | "SchemaExtension"'.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- 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 20 (ubuntu-latest) and GraphQL v16
- 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 (7)
.changeset/quick-chairs-cover.md (1)
1-7: LGTM! Clear and accurate changeset description.The changeset correctly describes the patch changes for supporting 'extend schema' syntax and adding 'link' to the built-in directives.
packages/import/tests/schema/fixtures/directive/h.graphql (1)
1-8: LGTM! Comprehensive test fixture for the new functionality.This fixture effectively demonstrates the key features being added:
- Schema extension with
@linkdirective- Custom directive definitions imported via
@link- Federation directives working with schema extensions
The fixture provides good test coverage for the import functionality.
packages/import/tests/schema/import-schema.spec.ts (1)
470-482: LGTM! Comprehensive test case for the new functionality.The test properly verifies that:
- Schema extensions with
@linkdirectives are correctly imported- Custom directives referenced in
@linkimports are processed- Federation directives work with schema extensions
The expected SDL output correctly matches the fixture, ensuring the import logic works as intended.
packages/import/src/index.ts (4)
33-33: LGTM! Correct import for schema extension support.Adding
SchemaExtensionNodeimport is necessary for handling schema extensions.
57-57: LGTM! Correct addition of 'link' to built-in directives.Adding 'link' to the
builtinDirectivesarray ensures it doesn't require explicit directive definitions in federated schemas, which aligns with the PR objectives.
321-323: LGTM! Correct handling of schema extensions.Adding the
SCHEMA_EXTENSIONcase that delegates tovisitSchemaExtensionDefinitionNodeproperly integrates schema extension processing into the visitor pattern.
819-825: LGTM! Proper implementation of schema extension visitor.The
visitSchemaExtensionDefinitionNodefunction correctly:
- Adds 'schema' dependency like schema definitions
- Visits directives to track directive dependencies
- Uses optional chaining for operation types (since they're optional in extensions)
This mirrors the behavior of
visitSchemaDefinitionNodeappropriately.
…#7225) * Support 'extend schema' syntax and add 'link' to the list of builtins * Fix typeecheck error * Use proposed type fix
Description
extend schemasyntax was ignored, causing it to not be included in the final imported schema.And
@linkerrors if included because of a missing directive definition.This change fixes both these issues so that a schema such as:
can be imported correctly so that the import result is a valid subgraph definition.
Related # (issue)
#7224
Type of change
Please delete options that are not relevant.
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
I opted for a minimal set of changes to resolve this issue. In the long run, we may consider what if federation is using a namespaced import e.g.
@federation__key