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

Add strawberry.context #1243

Closed
wants to merge 4 commits into from
Closed

Add strawberry.context #1243

wants to merge 4 commits into from

Conversation

jkimbo
Copy link
Member

@jkimbo jkimbo commented Sep 13, 2021

Description

This PR introduces strawberry.context which gives you access to the current context through the magic of Context Variables. I propose that becomes the recommend way to access the context rather than through info. What do you think @strawberry-graphql/core ?

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@jkimbo jkimbo requested a review from a team September 13, 2021 19:28
@codecov
Copy link

codecov bot commented Sep 13, 2021

Codecov Report

Merging #1243 (254854a) into main (5dbbb04) will decrease coverage by 0.57%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1243      +/-   ##
==========================================
- Coverage   97.63%   97.05%   -0.58%     
==========================================
  Files          88       89       +1     
  Lines        3382     3366      -16     
  Branches      496      495       -1     
==========================================
- Hits         3302     3267      -35     
- Misses         44       62      +18     
- Partials       36       37       +1     

async def get_user(self, info: Info, id: strawberry.ID) -> User:
return await info.context["user_loader"].load(id)
async def get_user(self, id: strawberry.ID) -> User:
return await strawberry.context["user_loader"].load(id)
Copy link
Member

Choose a reason for hiding this comment

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

do you think we can:

  1. type it somehow?
  2. use context.user_loader instead of context["user_loader"]

?

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. type it somehow?

I'm not sure how we could type it unfortunately. You can always cast it when you import it but that's not great. I think that's one of the downsides of this proposal.

  1. use context.user_loader instead of context["user_loader"]

That should already work. See tests.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, I wonder if doing:

ctx: Context[Something] = strawberry.context

is annoying or not.

Do we get benefits by using ContextVars and still using parameters (and dependency injection)?

Copy link
Member Author

@jkimbo jkimbo Sep 14, 2021

Choose a reason for hiding this comment

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

Do we get benefits by using ContextVars and still using parameters (and dependency injection)?

You can start defining (and exporting from Strawberry) stand alone functions that get useful things from the context. E.g. current_user() The only issue with that is there wouldn't be a way for those functions to be known about early in the execution to register things into the context before resolution happens. E.g. you couldn't do get_dataloader(UserDataloader) inside a resolver without also registering an extension on the schema that would inject the dataloader into the context. Dependency injection would allow you to that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh it would make the data validation proposal (#820 (comment)) a bit cleaner. Instead of having to pass info to the .validate() function you could grab the context from the context variable.

@AndrewIngram
Copy link
Contributor

Pros:

  1. Most common reason for needing the info arg is gone, so more more consistency in resolvers in general
  2. Doesn't feel like a mismatch given context is typically fairly static per query (mutations might update it, but regular queries shouldn't), whilst the rest of info differs per query+resolver.

Cons:

  1. Less explicit
  2. There's nothing stopping code further down the call stack from using strawberry.context either, which could result in some nasty coupling.

The second con is the one that most concerns me. I've seen Flask codebases where its magic request object is accessed at every conceivable point in the call stack and it's an absolute nightmare of tangled layers. As such, we'd want to discourage people from accessing strawberry.context outside of the "GraphQL" concern. Arguably the deepest it should go is Dataloaders (if that). There'd definitely need to be some "here be dragons" warnings needed in the docs.

But the fact that this guidance would make its utility mostly about simpler resolver signatures, it makes me wonder if there's enough of an upside to this? Perhaps there is though.

@jkimbo
Copy link
Member Author

jkimbo commented Sep 14, 2021

@AndrewIngram I'm beginning to wonder if there is much benefit to this as well. Especially because you can't type it effectively and the first Pro you list could be solved by #374

@jkimbo
Copy link
Member Author

jkimbo commented Sep 16, 2021

As discussed in our recent community meeting, I'm going to close this PR since we don't think it provides enough value to be worth the potential confusion.

@jkimbo jkimbo closed this Sep 16, 2021
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.

3 participants