Simplify operationTypes validation #999
Conversation
|
Thanks @IvanGoncharov. Will take a look. |
src/utilities/buildASTSchema.js
Outdated
| @@ -131,6 +131,7 @@ export function buildASTSchema(ast: DocumentNode): GraphQLSchema { | |||
| throw new Error('Must provide a document ast.'); | |||
| } | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
nit: remove whitespace
There was a problem hiding this comment.
@asiandrummer Done 👍
src/utilities/buildASTSchema.js
Outdated
| let queryTypeName; | ||
| let mutationTypeName; | ||
| let subscriptionTypeName; | ||
| let operationTypes = { |
There was a problem hiding this comment.
@asiandrummer It's intentional since on line 174:
operationTypes = {};This is because schema definition is optional but if it present you should use it instead on relaying on type names.
There was a problem hiding this comment.
const operationTypes = schemaDef ? { ... } : {};
Above seems better to me, but it's more of a personal pref to me - again it's a nit
There was a problem hiding this comment.
@asiandrummer Great idea 👍
I refactor out getOperationTypes function so it become:
const operationTypes = schemaDef ? getOperationTypes(schemaDef) : {
...
};
src/utilities/buildASTSchema.js
Outdated
| } | ||
|
|
||
| if (!queryTypeName) { | ||
| if (!operationTypes['query']) { |
There was a problem hiding this comment.
Query?
Also, we should probably create constants to avoid this type of typos.
| @@ -280,8 +255,8 @@ export function buildASTSchema(ast: DocumentNode): GraphQLSchema { | |||
| }); | |||
| } | |||
|
|
|||
| function getObjectType(typeNode: TypeDefinitionNode): GraphQLObjectType { | |||
| const type = typeDefNamed(typeNode.name.value); | |||
| function getObjectType(name: string): GraphQLObjectType { | |||
There was a problem hiding this comment.
Hmm, this is an API change. Have we done a sweep to see if we're breaking anything? is getObjectType only used in this file?
There was a problem hiding this comment.
@asiandrummer Yes, since it doesn't have export
src/utilities/buildASTSchema.js
Outdated
| subscription: subscriptionTypeName ? | ||
| getObjectType(nodeMap[subscriptionTypeName]) : | ||
| subscription: operationTypes['subscription'] ? | ||
| getObjectType(operationTypes['subscription']) : |
There was a problem hiding this comment.
Same as above. Am I misunderstanding something? It looks like we aren't being consistent with the operation names (query vs. Query and etc)
There was a problem hiding this comment.
@asiandrummer Fully remove strings now it: operationTypes.subscription.
consistent with the operation names (query vs. Query and etc)
It's how spec working, inside schema definition it's query operation but if schema definition is absent then you should look for Query type. So naming is following the specification.
IvanGoncharov
force-pushed
the
addNodesToErrors
branch
from
0339e5c to
d14fffe
Compare
@asiandrummer I try to break my changes into small PRs that can be fully reviewed. Right now I'm working on improving validation of SDL/IDL but before submitting my changes I want to simplify this function. Since if keep adding complexity to |
IvanGoncharov
force-pushed
the
addNodesToErrors
branch
from
d14fffe to
97b009d
Compare
|
@IvanGoncharov oh sorry, I meant to approve this PR a while ago. The changes look harmless - just wanted to double check that there are tests covering this part of the code, right? Thanks for the improvements/contribution as always :D |
@asiandrummer Yes, it's fully covered. Here is negative cases, positive cases are spread through the entire file so I can provide a single link for them. |
|
Nice work, @IvanGoncharov |
Simplify code and more importantly unify some of the exceptions.
@wincent Can you please review it?