Add 'isRequredArgument' & 'isRequredInputField' predicates

[IvanGoncharov]

No description provided.

[IvanGoncharov]

They make code more readable.

[mjmahone]

@IvanGoncharov is this something you're planning on re-using, or is it just to make this one location more readable?

I don't think it drastically improves readability, and there's some danger that isRequiredArgument would be implemented for a VariableDefinition instead of inside the Schema's Argument definition.

If you're going to use this elsewhere, it makes sense, otherwise we should leave things as-is.

[Marak]

required, not requred?

[IvanGoncharov]

@Marak Thanks a lot 👍 Fixed.

is this something you're planning on re-using, or is it just to make this one location more readable?

@mjmahone It's actually a very very painful topic. Initially I didn't want to overburden you with technical details but here is a full story.

So before graphql/graphql-spec#418 required === NonNull but after this change, you also need to check defaultValue. Lee made necessary changes in all major places including validation rules in #1274. But many other places including all external projects still assume that required === NonNull.

That makes me pay extra attention to all isNonNullType/isNullableType checks and at some point, we need to check all such places and fix all remaining bugs.
For example, here are a few places that are easy to guess:

graphql-js/src/utilities/findBreakingChanges.js

Lines 250 to 257 in 7cfd686

	if (isNonNullType(newArgDef.type)) {
	breakingChanges.push({
	type: BreakingChangeType.NON_NULL_ARG_ADDED,
	description:
	`A non-null arg ${newArgDef.name} on ` +
	`${newType.name}.${fieldName} was added`,
	});
	} else {

Adding non-null argument with a default value is not a breaking change.

graphql-js/src/type/validate.js

Lines 441 to 450 in 7cfd686

	if (!ifaceArg && isNonNullType(objectArg.type)) {
	context.reportError(
	`Object field argument ${object.name}.${fieldName}(${argName}:) ` +
	`is of required type ${inspect(objectArg.type)} but is not also ` +
	`provided by the Interface field ${iface.name}.${fieldName}.`,
	[
	getFieldArgTypeNode(object, fieldName, argName),
	getFieldNode(iface, fieldName),
	],
	);

Same problem here since according to the spec:
http://facebook.github.io/graphql/June2018/#sec-Objects

The object field may include additional arguments not defined in the interface field, but any additional argument must not be required, e.g. must not be of a non‐nullable type.

And probably many more places since
So these functions are not strictly necessary but they solve a lot of confusion and motivate people who don't necessarily know about graphql/graphql-spec#418 change to use correct check in their code.

[IvanGoncharov]

@mjmahone I just remembered that Lee made undefined a valid value for enums in b013e73 and that mean field.defaultValue === undefined is invalid check and instead it should use hasOwnProperty simular to this place:

graphql-js/src/type/definition.js

Line 1140 in 7cfd686

value: value.hasOwnProperty('value') ? value.value : valueName,

But I can't use hasOwnProperty since a lot of place incorrectly assign undefined to defaultValue. For example:

graphql-js/src/utilities/buildASTSchema.js

Line 311 in 7cfd686

defaultValue: valueFromAST(value.defaultValue, type),

I think it's even bigger reason to use dedicated function for this check.

[mjmahone]

@IvanGoncharov that makes sense, thank you for the backstory: it makes the motivation clear and agreement that this is desirable easy to reach.