Skip to content

Update comment for context to explain when a new context is created or a stale one is used #180

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

Closed
wants to merge 1 commit into from

Conversation

MartijnHols
Copy link

After investigating why my DataLoader was sending stale data when triggering an event over an AsyncIterator, I discovered that GraphQL uses the old context value that was created when the user subscribed. Because of this, caching needs to be disabled in DataLoader to avoid AsyncIterators from pushing data that may be hours old.

See graphql/graphql-js#894 for more info.

Explain when a new context is used during execution
@enisdenjo
Copy link
Owner

Hey there! Urm, I kinda' naturally expect the GQL context to be created only when starting the operation (query, mutation or subscription). Why would you expect the context to change on every value emission? Maybe I just dont understand the use case. The function signature works on par on all operations.

I think this documentation addition will just raise confusion, especially the part where you say:

GraphQL will always use the old context created when the user first subscribed.

There is no "old" context, the context you set on start is the only context for all operations.

Furthermore, over at graphql/graphql-js#894, it is understood that the contextValue cannot be modified after the execution started.

I think this is more a graphql-js semantic, there is nothing graphql-ws can do to change this; and once graphql-js adds support for dynamic contexts on each value exec in subscriptions - so will graphql-ws inherently.

@n1ru4l
Copy link
Contributor

n1ru4l commented May 10, 2021

I think adding this comment in a refined way makes sense as it is not obvious that a subscription context is not created per resolved value. Furthermore, I would say that this can be a potential source for memory leaks and caching issues, so giving the users a hint is always great.

@enisdenjo
Copy link
Owner

Sure @n1ru4l, I dont mind having a hint.

@MartijnHols, please refine the comment by simply saying that the context does not change on subscription value emission, and dont forget to run yarn gendocs.

@MartijnHols
Copy link
Author

Sorry for being so slow in getting back. Thanks for resolving it.

Hey there! Urm, I kinda' naturally expect the GQL context to be created only when starting the operation (query, mutation or subscription). Why would you expect the context to change on every value emission? Maybe I just dont understand the use case. The function signature works on par on all operations.

You're totally right, after I finally figured out what was happening and thought about it, it made sense. It's just that it was not obvious. When making a context-creator I didn't have a mental picture of the subscription model and never really considered it. In testing it worked for queries, mutations and the initial subscription so it didn't look broken either. Only later did I run into the issue of stale data being returned on value emission. It took a while before I figured out it was not my application or ORM caching (the mutation I discovered it in was triggered by the same user receiving the value emission) but in the value emission. I dove into the GraphQL code to figure out if there was some way I could configure a context for each value emission which led me to find the issue in the GraphQL repo mentioned above.

I think this is more a graphql-js semantic, there is nothing graphql-ws can do to change this; and once graphql-js adds support for dynamic contexts on each value exec in subscriptions - so will graphql-ws inherently.

Indeed, the proposed is just in the hope that the proposed/merged comment will help someone figure this out easier or prevent the issue altogether. In implementing subscriptions thanks to this excellent library developers work with graphql-ws, not graphql-js, so I think developers would benefit from proxying the info.

@enisdenjo
Copy link
Owner

No worries! I understand your point and you're absolutely right. I hope c6becd2 will help others figure the details faster and point them in the right direction.

@enisdenjo
Copy link
Owner

🎉 This issue has been resolved in version 4.5.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@enisdenjo enisdenjo added the released Has been released and published label May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Has been released and published
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants