Fix handling of core/link when definitions are provided or partially so #1662
Conversation
👷 Deploy request for apollo-federation-docs pending review.Visit the deploys page to approve it
|
|
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. |
pcmanus
force-pushed
the
fix-expanding-core
branch
from
0848af3 to
de7470c
Compare
| @@ -1499,6 +1509,10 @@ export class SchemaDefinition extends SchemaElement<SchemaDefinition, Schema> { | |||
| const imports = extractCoreFeatureImports(url, schemaDirective); | |||
| const core = new CoreFeature(url, args.as ?? url.name, schemaDirective, imports, args.for); | |||
| Schema.prototype['markAsCoreSchema'].call(schema, core); | |||
| // We also any core features that may have been added before we saw the @link for link itself | |||
There was a problem hiding this comment.
i am not 100% sure i understand what case this is handling, so maybe disregard, but the @core and @link specs both require that the bootstrap @link is the first one in the document; any prior @links should be ignored.
There was a problem hiding this comment.
Yeah, that's the an artifact of how the code works right now. Essentially, because the abstraction is mutable, the way incomplete schema are expanded is done by taking the input and adding what's missing. So when we get an input that only has a @link to the federation spec, but no @link-to-link-itself, we end up adding that @link-to-link-itself to a document hat already has another existing @link and this code ensure that pre-existing feature is still tracker correctly.
To be clear, the resulting "final" document still end up with the @link-to-link as the first one on the SchemaDefinition, the code is just need due to way incomplete schema are auto-completed.
the
@coreand@linkspecs both require that the bootstrap@linkis the first one in the document
As a minor aside, this made me notice that the current core/v0.2 spec (https://specs.apollo.dev/core/v0.2/) only ever talk about SchemaDefinition in the bootstrap/feature collection descriptions. Since we've making extensive doc of putting @link on SchemaExtension in our docs/examples, we should make sure to clarify their handling.
I'm afraid it's not that simple because we've had a few Fwiw, my personal opinion is that since |
internals-js/src/coreSpec.ts
Outdated
|
|
||
| private createDefinitionArgumentSpecifications(schema: Schema, nameInSchema?: string): { args: ArgumentSpecification[], errors: GraphQLError[] } { | ||
| const args: ArgumentSpecification[] = [ | ||
| { name: this.urlArgName(), type: new NonNullType(schema.stringType()) }, |
There was a problem hiding this comment.
can we weaken this to just String for the moment? i'd like v1.0 to maintain universality. there are a few mechanisms to do this, but @link(url: null, import: "looks__namespacedButIsnt") is one way to handle it, and i'd like us not to have painted ourselves into a corner by shipping it like this.
There was a problem hiding this comment.
Make sense. And was simple-enough, so made the change in the PR (with a test to double-check it's a safe change, that is that we were still correctly reading previously generated definition having the non-nullable type).
pcmanus
force-pushed
the
fix-expanding-core
branch
from
de7470c to
ee7ac74
Compare
This fixes the issue described on #1657 but also fix some related handling of errors (that were not properly surfaced previously). I made a separate PR because the issue was not so much with composition than with subgraph validations, so I preferred moving tests in the existing test file dedicated to those subgraph validations. But also, the repro from #1657 uses some definitions that are not exactly the ones that have been used so far and so it doesn't pass for those reasons.