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

Subscriptions RFC: How to pass payload data into subscription resolvers? #280

Closed
robzhu opened this issue Mar 6, 2017 · 4 comments
Closed

Comments

@robzhu
Copy link
Contributor

robzhu commented Mar 6, 2017

How should we pass event payloads/data into the subscription resolvers?
@stubailo @taion @leebyron @dschafer @wincent

Continuing the conversation from: #267 (comment)

@stubailo
Copy link
Contributor

stubailo commented Mar 6, 2017

So, here are some options:

  1. As parent/root object
  2. As an extra fake argument in the same place as query args
  3. Attached to context
  4. As a new argument to the resolver signature

I think 3 wouldn't be great, because I don't think non-root resolvers should have access to the payload.

2 also seems weird, although it's the least "breaking" of these options.


That seems to leave us with:

  • (1) Pass the payload as the root object, which might conflict with some other ways people are using this object today, and
  • (4) Introduce a new argument to the resolver signature, and make it resolve(parent, args, context, payload, [info]) which is a big change

For passing as the root object, the extra information we want to gather is how people are using this parameter today. For myself personally, I put everything on the context, but I've heard some people have opinions about how some common data should go on rootValue instead. So in my world passing as rootValue is just fine, but if people are already using that for something important then it might not be great.

Passing as an extra parameter also comes with some questions. Does every resolver get this extra argument, or do subscription resolvers get a different number of arguments than others? Also, this could end up being a pretty big breaking change for the GraphQL execution libraries, since the arguments will end up moving around.

I suppose there is another option - the spec says it's an "extra parameter", but the actual implementations put it somewhere implementation-specific, for example as a property on info in graphql.js. Then we could avoid the breaking aspects of changing rootValue, but at the expense of making the spec a little less descriptive and opening up to differences in language implementations.

@taion
Copy link

taion commented Mar 6, 2017

I'd prefer rootValue, because I think for modern GraphQL.js, using rootValue instead of context for context seems like an anti-pattern.

But I think this is entirely an implementation-level question. The GraphQL spec doesn't talk about things like context, for example.

@robzhu robzhu added the RFC label Mar 29, 2017
@robzhu
Copy link
Contributor Author

robzhu commented Apr 28, 2017

rootValue it is. Since this is specific to the GraphQL-js implementation, it will be left out of the spec.

@robzhu robzhu closed this as completed Apr 28, 2017
@tcr
Copy link

tcr commented Apr 28, 2017

I see the tradeoff similar to how @taion describes it. While #267 appropriately calls out that this can be confusing if resolvers are relying on rootValue to be consistent, in my experience so far working with subscriptions this isn't an issue that you make once or more than once. Let's ensure the RFC doesn't conflict with implementations which chose to do this.

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

4 participants