fix #1277 ensure interface has at least 1 concrete type #1280
Conversation
src/type/validate.js
Outdated
| if (possibleTypes.length === 0) { | ||
| context.reportError( | ||
| `No concrete types found for Interface type ${iface.name}. ` + | ||
| `If only referenced via abstraction, add concrete types to schema.types array`, |
There was a problem hiding this comment.
The schema can be create using SDL syntax so add concrete types to schema.types array part would be confusing in such case.
concrete types - better to avoid inventing new terminology.
Something like this would be great:
Interface ${iface.name} should be implemented by at least one object type.
Disclaimer: I'm grammatically challenged 😄
Matching PR for graphql-js: graphql/graphql-js#1280
|
@IvanGoncharov ready for another review! |
src/type/schema.js
Outdated
| @@ -204,7 +207,7 @@ export class GraphQLSchema { | |||
| if (!possibleTypeMap[abstractType.name]) { | |||
| const possibleTypes = this.getPossibleTypes(abstractType); | |||
| invariant( | |||
| Array.isArray(possibleTypes), | |||
| Array.isArray(possibleTypes) && possibleTypes.length > 0, | |||
There was a problem hiding this comment.
This is a late checking invariant, not part of the scheme validation. This change shouldn't be necessary
There was a problem hiding this comment.
@leebyron this was only necessary for getting the schema tests working
There was a problem hiding this comment.
The schema tests will likely need to change instead of changing this invariant. In fact, this entire invariant check shouldn't even be necessary if this step is done as part of schema validation
There was a problem hiding this comment.
Fully agree. The invariant can go away because getPossibleTypes is now guaranteed to return an array.
The test can go away because there's no way isPossibleType can produce an error.
| @@ -76,19 +76,6 @@ const Schema = new GraphQLSchema({ | |||
| }); | |||
|
|
|||
| describe('Type System: Schema', () => { | |||
| describe('Getting possible types', () => { | |||
There was a problem hiding this comment.
isPossibleType can no longer throw, so this test is no longer needed
| @@ -159,6 +160,8 @@ export class GraphQLSchema { | |||
| this._implementations[iface.name] = [type]; | |||
| } | |||
| }); | |||
| } else if (isAbstractType(type) && !this._implementations[type.name]) { | |||
| this._implementations[type.name] = []; | |||
There was a problem hiding this comment.
guarantees that getPossibleTypes returns an array, as it should per its flow type.
|
@leebyron that should fix it up |
src/type/validate.js
Outdated
|
|
||
| if (possibleTypes.length === 0) { | ||
| context.reportError( | ||
| `Interface ${iface.name} must be implemented` + |
There was a problem hiding this comment.
Could you please expand the tests further to include a test which asserts this error occurs?
|
validation test added, ready for another go |
|
Great work! |
* Require Interface to implement at least 1 Object Matching PR for graphql-js: graphql/graphql-js#1280 * add period
Changes:
getPossibleTypesis now guaranteed to return an array, just like its flow typing says it does