fix(compostition): Fix port of @composeDirective#9164
fix(compostition): Fix port of @composeDirective#9164sachindshinde wants to merge 4 commits intodevfrom
@composeDirective#9164Conversation
✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: 983cd304143e46a4dd01dfb7 ✅ AI Style Review — No Changes DetectedNo MDX files were changed in this pull request. Review Log: View detailed log
|
sachindshinde
force-pushed
the
sachin/fix-compose-directive
branch
from
5a5c008 to
be72cb3
Compare
|
@sachindshinde, please consider creating a changeset entry in |
sachindshinde
force-pushed
the
sachin/fix-compose-directive
branch
from
3a8755d to
a532747
Compare
sachindshinde
force-pushed
the
sachin/fix-compose-directive
branch
2 times, most recently
from
b5eeef8 to
5dcf94a
Compare
Co-authored-by: apollo-mateuswgoettems <[email protected]>
sachindshinde
force-pushed
the
sachin/fix-compose-directive
branch
from
5dcf94a to
b809f3d
Compare
dariuszkuc
left a comment
dariuszkuc
left a comment
There was a problem hiding this comment.
Looks good just a couple questions around tests
| ); | ||
|
|
||
| let result = compose(vec![subgraph_a, subgraph_b]).unwrap(); | ||
| println!("{:?}", result.hints()); |
| pub spec_alias: Option<Name>, | ||
| pub imports: Vec<Arc<Import>>, | ||
| pub purpose: Option<Purpose>, | ||
| pub line_column_range: Option<Range<LineColumn>>, |
There was a problem hiding this comment.
note for self: this is added so we can calculate location information for subgraph errors
| let imports = directives | ||
| .iter() | ||
| .map(|(original, alias)| { | ||
| .map(|(alias, original)| { |
| @@ -1687,18 +1684,14 @@ format!("Field \"{field}\" of {} type \"{}\" is defined in some but not all subg | |||
| // We should skip the supergraph specific directives, that is the @core and @join directives. | |||
There was a problem hiding this comment.
can you update this comment to @link and @join directives?
| ); | ||
| let subgraph_b = generate_subgraph( | ||
| "subgraphB", | ||
| r#"@link(url: "https://specs.custom.dev/foo/v1.1", import: ["@bar"])"#, |
There was a problem hiding this comment.
just wondering whether we should be explicit with import: ["@bar", "@foo"] here (or maybe use different custom directive names as @foo is implicitly handled as its name matches the spec name)
| r#"@composeDirective(name: "@foo")"#, | ||
| r#" | ||
| directive @foo(name: String!) on FIELD_DEFINITION | ||
| directive @bar(name: String!) on FIELD_DEFINITION |
There was a problem hiding this comment.
should this only include @foo definition and skip the @bar (and vice versa in subgraph_b)?
| r#"@composeDirective(name: "@foo")"#, | ||
| r#" | ||
| directive @foo(name: String!) on FIELD_DEFINITION | ||
| directive @bar(name: String!) on FIELD_DEFINITION |
2 participants
While reviewing #8936, I noticed some issues in the PR, but there were also more issues in the surrounding code for
@composeDirective. This PR brings the Rust port more inline with the JS code (including the changes that #8936 were trying to port over; the tests from that PR have been pulled into this one). Note that the tests overall seem to be a bit different, and I'm still in the middle of double-checking them, but the non-test code should be ready for review.