Use empty arrays instead of undefined properties

[IvanGoncharov]

This PR makes extensionASTNodes and __allowedLegacyNames fields to always be arrays similar to other places in this lib:

graphql-js/src/type/definition.js

Line 619 in 499a759

const interfaces = resolveThunk(interfacesThunk) || [];

graphql-js/src/type/definition.js

Line 947 in 499a759

const types = resolveThunk(typesThunk) || [];

[leebyron]

I'd prefer to see no defined property instead of empty arrays when extension is never attempted - this should avoid allocations for larger schema and keep debugging views simpler

[IvanGoncharov]

I'd prefer to see no defined property instead of empty arrays when extension is never attempted - this should avoid allocations for larger schema and keep debugging views simpler

@leebyron Valid points. That's why I would never suggest doing something like this for AST nodes. However, in case of GraphQL schema I think they are less critical:

keep debugging views simpler

It doesn't affect console.log since those objects have custom toString

graphql-js/src/type/definition.js

Lines 603 to 608 in 499a759

	toString(): string {
	return this.name;
	}
	toJSON: () => string;
	inspect: () => string;

should avoid allocations for larger schema

It's definitely not a problem for __allowedLegacyNames since it occurs only once per schema. But it valid point for extensionASTNodes however I would argue that it's overweighted by the following arguments:

• consistency with other fields, e.g. interfaces and `types
• makes the code simpler, e.g. you don't need to write this:

  return type.astNode
    ? type.extensionASTNodes
      ? [type.astNode].concat(type.extensionASTNodes)
      : [type.astNode]
    : type.extensionASTNodes || [];

[mjmahone]

I'd probably also (slightly) prefer to keep extensions undefined for empty: there might be hundreds or thousands of different GraphQLObject objects in a schema, and especially in gigantic schemas, you are unlikely to extend any specific object: I'd argue it tends to happen on <1% of types. I see the point about it making the code simpler, and usually would agree with you that it's worth it.

On the other hand, for interfaces, the larger your schema is, the more likely a type is to implement an interface. However, if there's a concrete example where type.extensionASTNodes is getting used a lot (and converted to [] when empty a lot, so allocating memory anyways), I'd approve this. I don't think the use case is that common, though.

[IvanGoncharov]

I'd argue it tends to happen on <1% of types

@mjmahone Agree. I removed extensionASTNodes changes from this PR so know it only affects __allowedLegacyNames.

However, if there's a concrete example where type.extensionASTNodes is getting used a lot (and converted to [] when empty a lot, so allocating memory anyways), I'd approve this. I don't think the use case is that common, though.

Majority of the issues would be solved by #1331

[mjmahone]

I really like what's happening in #1331 so I'd personally advocate merging that and closing this one.

[IvanGoncharov]

@mjmahone I agree with your arguments about extensionASTNodes so I reverted this part but I don't see why __allowedLegacyNames can't be an empty array.

[mjmahone]

Yup I agree: __allowedLegacyNames only lives on the hopefully only-one-instance Schema object, so that's a good change.