Skip to content
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.

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

Interface fields are not generic #720

Closed
mike-marcacci opened this issue Feb 16, 2017 · 4 comments
Closed

Interface fields are not generic #720

mike-marcacci opened this issue Feb 16, 2017 · 4 comments

Comments

@mike-marcacci
Copy link
Contributor

mike-marcacci commented Feb 16, 2017

Hi there! My apologies if this has been covered elsewhere, but this is something I was very surprised to find missing from GraphQL-js, even after reading the full GraphQL spec. I want to make sure I'm not missing something, and this is in fact impossible.

When an interface is defined with interface-type fields, I assumed that implementations of the parent interface could specify implementations of the child interface in the corresponding field. Any queries that are generic over the parent interface would function as they currently do with inline fragments (since that's the highest level of specificity guaranteed); queries that resolve to an instance of the parent interface which further specifies the field in question should be able to use the child's fields directly.

This would go a long way in codifying conventions that are currently expressed out-of-channel. Let's take the Relay specifications for example. While Node can be defined as an interface, Edge and Connection are just conventions, which feels quite brittle.

Instead, I would have expected this to be possible:

export const Node = new GraphQLInterfaceType({
	name: 'Node',
	description: 'A resource with an ID. Required by the Relay specification.',
	fields: () => ({
		id: { type: new GraphQLNonNull(GraphQLID) }
	}),
	resolveType: () => {
		// ...
	},
});


export const Edge = new GraphQLInterfaceType({
	name: 'Edge',
	description: 'A wrapper around a node that supplies a cursor. This formalizes a requirement of the Relay specification.',
	fields: () => ({
		cursor: { type: GraphQLString },
		node: { type: Node },
	}),
	resolveType: () => {
		// ...
	},
});

export const User = new GraphQLObjectType({
	name: 'User',
	interfaces: () => [Node],
	fields: () => ({
		id: { type: new GraphQLNonNull(GraphQLID) },
		name: { type: GraphQLString }
	}),
});

export const UserEdge = new GraphQLObjectType({
	name: 'UserEdge',
	interfaces: () => [Edge],
	fields: () => ({
		cursor: { type: GraphQLString },
		node: { type: User },
	}),
});

Because User implements Node, then UserEdge should definitely satisfy the requirements of Edge. However, we get the following error:

Error: Edge.node expects type "Node" but UserEdge.node provides type "[User]".

Is this intentionally omitted functionality? Is there a fundamental reason this couldn't work, or is this something worth formally proposing?

Thanks in advance!

@mike-marcacci
Copy link
Contributor Author

As I've continued working on my current project, I've grown increasingly convinced that adding this ability would dramatically enhance the expressiveness of GraphQL as a spec, without any serious downside that's immediately obvious to me.

I realize that this is probably a topic that extends beyond the scope of graphql-js as a library... is there a more appropriate place I can take this idea?

Thanks!

@mike-marcacci
Copy link
Contributor Author

mike-marcacci commented Apr 3, 2017

In case anyone finds this issue looking for similar functionality, I've moved my proposed solutions to the GraphQL language repo:

graphql/graphql-spec#294
graphql/graphql-spec#295

I'll leave the issue open, but won't be offended if somebody on the graphql-js team feels it should be closed :-)

@mike-marcacci
Copy link
Contributor Author

Actually... per graphql/graphql-spec#294 (comment) it appears that the behavior I expected is correct according to the Object type validation rules... so I guess this is actually a bug.

@mike-marcacci
Copy link
Contributor Author

Nope... not a bug, as the test is working as expected. Sorry for all the noise.

I'll go back and see what other factors may have made this error appear, and reopen if I can reproduce. Again, sorry for the noise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant