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

ExecutionOptions.UserContext: object -> IDictionary<string, object> #218

Merged

Conversation

sungam3r
Copy link
Member

draft fix for #217

using System.Threading.Tasks;

namespace GraphQL.Server.Transports.Subscriptions.Abstractions
{
public class MessageHandlingContext : IDisposable
public class MessageHandlingContext : Dictionary<string, object>, IDisposable
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about that. Inside there is already a ConcurrentDictionary for some properties. Not yet thoroughly understood this repository.

@pekkah
Copy link
Collaborator

pekkah commented Mar 28, 2019

Because originally user context was object we needed to hijack it so we could add more properties into it. That's why on server side it already is a dictionary. The original user context is injected into it and available on resolvers.

@sungam3r
Copy link
Member Author

Need to make some more fixes?

@joemcbride
Copy link
Member

@sungam3r Would you mind fixing this merge conflict and I'll get this merged? There may be some other refactoring that we need to do for subscriptions, though this should at least get the core project working.

@sungam3r
Copy link
Member Author

sungam3r commented Jun 4, 2019

Yes, I want to do it, but I did not understand the comment from @pekkah

Because originally user context was object we needed to hijack it so we could add more properties into it. That's why on server side it already is a dictionary. The original user context is injected into it and available on resolvers.

I suspect I'm doing something wrong. I will come back to this a little later, I need to figure it out.

@joemcbride
Copy link
Member

I suspect I'm doing something wrong.

No, it was how it was designed. Subscriptions needed to pass along some additional metadata. @pekkah accomplished that with the UserContext. But, that would override the existing UserContext. So he devised a way to work around that. Now that UserContext is a Dictionary this can be refactored. I can do that work, I just want to get what you have accomplished so far committed.

@sungam3r
Copy link
Member Author

sungam3r commented Jun 4, 2019

You can do it yourself if it is required as quickly as possible. I still need some time to figure it out myself. I do not want to commit things if I do not fully understand what I have done. Now honestly, I am very tired, I can only return to this tomorrow.

@joemcbride
Copy link
Member

No rush. In fact, it looks like there is only one file conflicting. I'll take care of it.

@joemcbride joemcbride merged commit cee670d into graphql-dotnet:develop Jun 4, 2019
@sungam3r sungam3r deleted the sync-with-graphql-prerelease branch June 4, 2019 23:23
@joemcbride
Copy link
Member

@sungam3r good news, subscriptions still work with those changes. 🎉

@sungam3r
Copy link
Member Author

sungam3r commented Jun 5, 2019

Good

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

Successfully merging this pull request may close these issues.

None yet

3 participants