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

[RFC] Pass an object to resolvers rather than four individual arguments #844

Closed
mxstbr opened this issue May 8, 2017 · 7 comments
Closed

Comments

@mxstbr
Copy link

mxstbr commented May 8, 2017

In graphql-js the argument signature for a resolver looks like this:

(root, args, context, info) => ...

Not every resolver cares about all of these arguments though. In fact, I think it's fair to say most resolvers don't care about all of these arguments. Even the documentation agrees: (source, emphasis mine)

obj: The previous object, which for a field on the root Query type is often not used.

obj, the first argument to a resolver, is often not used?

In my codebase I frequently find myself writing code like this where I skip one or more arguments:

(_, __, { user }) => ...
(_, { first }) => ...
(book, _, { user }) => 

In JavaScript we don't have named arguments like in other languages, but since the introduction of ES2015 we have something close enough: destructuring.

Rather than passing in four individual arguments I think it would be much better if resolvers got an object with these four arguments as properties. This would look like this from a users perspective:

({ root, args, context, info }) => ...

For the above three examples the code becomes a lot neater:

({ context: { user } }) => ...
({ args: { first } }) => ...
({ root, context: { user } }) => 

Not only does the code become neater, it also becomes clearer and easier to read. Even as somebody who's using GraphQL in production, to understand the first examples I have to read the code multiple times to grasp what exactly is going on. For the second examples I can read it and know immediately what's going on.

This is only multiplied for people who aren't intimately familiar with GraphQL. It's clear what's going on, whereas the first functions are relatively confusing.

Is there any interest in releasing a breaking change and moving towards a single object argument? In my opinion it would enhance the API a lot, I couldn't find any previous discussions about why it is the way it is though so I might be missing something. It should also be possible to write a codemod for the conversion, so users wouldn't need to feel left behind if they had a large codebase.

I'm unsure if this is a spec concern or a graphql-js concern, feel free to move this issue over to the spec if this is irrelevant to this implementation.

Related discussion (about the graphql function): #356

@mxstbr mxstbr changed the title Pass an object to resolvers rather than four individual arguments [RFC] Pass an object to resolvers rather than four individual arguments May 8, 2017
@stubailo
Copy link

stubailo commented May 8, 2017

One question is whether this can be done in a backwards compatible manner, and if that matters. Given the number of different tools written around GraphQL.js, it would be unfortunate to break all of them. On the other hand, I strongly support this API change and would be in favor of moving all argument lists with more than 2 arguments to take objects.

@mxstbr
Copy link
Author

mxstbr commented May 8, 2017

One question is whether this can be done in a backwards compatible manner, and if that matters. Given the number of different tools written around GraphQL.js

I don't think that matters.

In fact, I think there's an argument to be made that it'd be better to release this as a breaking change sooner rather than later. (assuming the maintainers agree this is a good change) GraphQL is in the early adopter stage, (or maybe even innovators stage?) so breaking changes like this to refine the API are still possible. Down the line I doubt it'll be as easy as it'd be now.

@kkemple
Copy link

kkemple commented May 8, 2017

I also agree, I think object structure would be better and since GraphQL is so new it would be better to rip the bandaid off sooner rather than later. Perhaps a codemod could be made as well to help with migrations for any who need to upgrade.

@leebyron
Copy link
Contributor

leebyron commented May 9, 2017

We're not going to issue a breaking change to the resolver API.

I disagree that this change would result in a more desirable API. I think you're focusing on edge cases and on your own particular style of writing code which may not be as familiar to more developers. Our goal was to create an idiomatic JavaScript API and requiring functions that take objects with named keys as arguments is not a common usage. Destructuring isn't common practice yet either.

I agree that the vast majority of resolvers do not care about all four arguments. Typically they only rely on the first: source. Destructuring would require additional punctuation for little value.

The arguments to the resolver functions are more or less in the order of usefulness, with the vast majority of resolvers being a function of the source value (first arg) and sometimes also args, and in rare cases context or execution info as well.

I'm happy to continue conversation on this, and discuss ways to improve the API in a non-breaking way

@leebyron leebyron closed this as completed May 9, 2017
@leebyron
Copy link
Contributor

leebyron commented May 9, 2017

I'll also make a note to update the documentation for resolvers which I think is misleading. In fact the source value is almost always useful, even on the top level query type, to access values from the rootValue

@stubailo
Copy link

stubailo commented May 9, 2017

While I don't think making this kind of API change is super important, I disagree that destructuring or named arguments isn't common. Especially since GraphQL with Node is mostly adopted by people who are already using some modern JavaScript tooling, I think most people would see this change as a benefit.

@marano
Copy link

marano commented Jun 3, 2018

Four arguments makes it very difficult to compose resolvers. I am sad to see this proposal was dismissed without much discussion.

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

5 participants