Skip to content

Unify optional callbacks validation #1411

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 29, 2018
Merged

Unify optional callbacks validation #1411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions src/type/__tests__/definition-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -730,7 +730,9 @@ describe('Type System: Interface types must be resolvable', () => {
resolveType: {},
fields: { f: { type: GraphQLString } },
}),
).to.throw('AnotherInterface must provide "resolveType" as a function.');
).to.throw(
'AnotherInterface must provide "resolveType" as a function, but got: {}.',
);
});
});

Expand Down Expand Up @@ -782,7 +784,9 @@ describe('Type System: Union types must be resolvable', () => {
types: [ObjectWithIsTypeOf],
}),
),
).to.throw('SomeUnion must provide "resolveType" as a function.');
).to.throw(
'SomeUnion must provide "resolveType" as a function, but got: {}.',
);
});
});

Expand Down Expand Up @@ -905,7 +909,9 @@ describe('Type System: Object types must be assertable', () => {
fields: { f: { type: GraphQLString } },
}),
);
}).to.throw('AnotherObject must provide "isTypeOf" as a function.');
}).to.throw(
'AnotherObject must provide "isTypeOf" as a function, but got: {}.',
);
});
});

Expand Down
40 changes: 16 additions & 24 deletions src/type/definition.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,12 +651,11 @@ export class GraphQLObjectType {
this._fields = defineFieldMap.bind(undefined, config);
this._interfaces = defineInterfaces.bind(undefined, config);
invariant(typeof config.name === 'string', 'Must provide name.');
if (config.isTypeOf) {
invariant(
typeof config.isTypeOf === 'function',
`${this.name} must provide "isTypeOf" as a function.`,
);
}
invariant(
config.isTypeOf == null || typeof config.isTypeOf === 'function',
`${this.name} must provide "isTypeOf" as a function, ` +
`but got: ${inspect(config.isTypeOf)}.`,
);
}

getFields(): GraphQLFieldMap<*, *> {
Expand Down Expand Up @@ -724,7 +723,7 @@ function defineFieldMap<TSource, TContext>(
name: fieldName,
};
invariant(
isValidResolver(field.resolve),
field.resolve == null || typeof field.resolve === 'function',
`${config.name}.${fieldName} field resolver must be a function if ` +
`provided, but got: ${inspect(field.resolve)}.`,
);
Expand Down Expand Up @@ -757,11 +756,6 @@ function isPlainObj(obj) {
return obj && typeof obj === 'object' && !Array.isArray(obj);
}

// If a resolver is defined, it must be a function.
function isValidResolver(resolver: mixed): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we keep isValidResolver? I'm not totally sure what the advantage of inlining it is

Copy link
Member Author

@IvanGoncharov IvanGoncharov Jul 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One reason would be that you can't share it without changing the name since isTypeOf not a resolver. Plus it also confusing since null and undefined are not valid resolvers but an explicit absence of resolver.

So the best name that I come up is isOptionalFunction which is the same as to just write resolver == null || typeof resolver === 'function'.
I think it's better to stay explicit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a reasonable argument. I'm convinced.

return resolver == null || typeof resolver === 'function';
}

export type GraphQLObjectTypeConfig<TSource, TContext> = {|
name: string,
interfaces?: Thunk<?Array<GraphQLInterfaceType>>,
Expand Down Expand Up @@ -903,12 +897,11 @@ export class GraphQLInterfaceType {
this.resolveType = config.resolveType;
this._fields = defineFieldMap.bind(undefined, config);
invariant(typeof config.name === 'string', 'Must provide name.');
if (config.resolveType) {
invariant(
typeof config.resolveType === 'function',
`${this.name} must provide "resolveType" as a function.`,
);
}
invariant(
config.resolveType == null || typeof config.resolveType === 'function',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these could now become isValidResolver

`${this.name} must provide "resolveType" as a function, ` +
`but got: ${inspect(config.resolveType)}.`,
);
}

getFields(): GraphQLFieldMap<*, *> {
Expand Down Expand Up @@ -981,12 +974,11 @@ export class GraphQLUnionType {
this.resolveType = config.resolveType;
this._types = defineTypes.bind(undefined, config);
invariant(typeof config.name === 'string', 'Must provide name.');
if (config.resolveType) {
invariant(
typeof config.resolveType === 'function',
`${this.name} must provide "resolveType" as a function.`,
);
}
invariant(
config.resolveType == null || typeof config.resolveType === 'function',
`${this.name} must provide "resolveType" as a function, ` +
`but got: ${inspect(config.resolveType)}.`,
);
}

getTypes(): Array<GraphQLObjectType> {
Expand Down