First crack at adding the ability to merge core directives. #1996
Conversation
Note that code is not very clean. All skipped/todo tests are not working and will require a minor refactor console.log etc. still exist with a few TODOs
✅ Deploy Preview for apollo-federation-docs canceled.
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
There was a problem hiding this comment.
I understand it's an unfinished patch, but left a number of early comments since I'm not sure how much I'll be able to look at future iterations.
Overall, the general approach looks good to me and it looks like a great early version :). My most important remarks are probably around turning the usages of @composeDirective we don't support into hard errors instead of just ignoring them with hints, because I worry about this creating surprises later when we do support those usages.
| const result = composeServices([subgraphA, subgraphB]); | ||
| expect(errors(result)).toEqual([]); | ||
| expect(hints(result)).toStrictEqual([ | ||
| ['CORE_DIRECTIVE_MERGE_INFO', 'Composed directive "@foo" named inconsistently (subgraph, directiveName). ("subgraphA","@foo"),("subgraphB","@bar")'], |
There was a problem hiding this comment.
Fwiw, I'd be personally ok to not even hint in that case. That is, on the one side, I get that mentioning the inconsistent naming could serve as a warning that this will get in the way if you want to compose it later. On the other side, I still worry about the value of hints diminishing if we generate too much of them, and I think the value of this one is debatable.
But if we do produce that hint, then I would suggest that:
- we make it a low level. At least not WARN, because there is no issue with that hint if you know what you're doing and I still think that a number of people take warnings as "call for action".
- I'd add more information in the message as to why raising this as a hint make sense in the first place, because that may not obvious to all users. In this case, as said above, this may be something to look at "if you ever intend to compose that directive" and this because the later currently require consistent naming.
There was a problem hiding this comment.
I'm assuming that this is meant to refer to the "core directive named differently in different subgraphs results in hint if not composed" test, but correct me if I'm wrong.
So get rid of the hint when neither is composed.
Hint at Warn level if one is composed
Error if both are composed.
I'm happy to get rid of this hint. I think going back to what you said initially is that really if it's not composed anywhere, it's probably just a directive that will never be composed is the right way of thinking about it.
… erroring for a specific message
…ive doesn't export it.
…ndant code elimination
…an a warning. Also wrote a few more tests.
|
@clenfest |
|
@FanfanYoung92 So the schema on the request context is the "API Schema", which is the schema presented to clients. It's not the "Supergraph schema" with all the metadata for query planning, or any custom directives. You can access the supergraph schema on the Apollo Gateway instance: https://stackblitz.com/edit/basic-federation-2-fkfwme?file=gateway.js |
Note that code is not very clean.
All skipped/todo tests are not working and will require a minor refactor
console.log etc. still exist with a few TODOs