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

feat: support custom dependency scopes specified outside of Xpresso #97

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

adriangb
Copy link
Owner

No description provided.

@adriangb
Copy link
Owner Author

@florimondmanca does this reflect your use case from #96?

I think this works but is pretty unergonomic. There's a lot of footguns:

  1. Making sure the same scope is used for the dependency everywhere. Probably can be alleviated by using type aliases (FooDep = Annotated[Foo, Depends(scope=...)].
  2. The way I set it up the Foo instance is cached in the "global" scope because di recognizes the dependency based on it's call parameter (in this case the class Foo). This brakes if you use a lambda to bind (lambda: some_foo_instance != Foo).

Any suggestions are welcome 😄

@codecov-commenter
Copy link

codecov-commenter commented May 16, 2022

Codecov Report

Merging #97 (9f1a339) into main (576ce20) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #97      +/-   ##
==========================================
- Coverage   98.57%   98.56%   -0.02%     
==========================================
  Files         126      126              
  Lines        4218     4248      +30     
  Branches      594      597       +3     
==========================================
+ Hits         4158     4187      +29     
- Misses         28       29       +1     
  Partials       32       32              
Impacted Files Coverage Δ
xpresso/routing/operation.py 100.00% <ø> (ø)
xpresso/routing/websockets.py 92.59% <ø> (ø)
...sts/test_dependencies/test_dependency_injection.py 100.00% <100.00%> (ø)
xpresso/applications.py 96.06% <100.00%> (+0.03%) ⬆️
xpresso/binders/_binders/union.py 90.62% <0.00%> (-1.57%) ⬇️

@florimondmanca
Copy link

florimondmanca commented May 17, 2022

@adriangb Hi! I must say I'm not sure I can make an informed comment on this, as I'm not sure I really understand di's notion of scopes. Also, as I wrote in a late comment on #96, my use case would currently be satisfied, since I prefer / get away with the non-type-annotated DI style. Xpresso's DI works for web components and that's very much enough.

Though, the example shown in the test looks like it would allow me to use type hints as a DI mechanism in the web part. But the fact that it induces coupling by requiring passing the scope everywhere we need the dependency does seem like a smell.

I actually view things as two separate systems. In DDD terms, I've got my domain/application layer where I do DI with one container - be it with type hints or a resolve() function style. And then there's the web layer which belongs to the infrastructure and that's got its own container for handling web stuff. Regardless of the style, I wonder whether I should keep these two containers separate anyway. This sparks a range of questions when using an approach like di's (DAG, multiple scopes, etc), such as could two DAGs live one intertwined with the other, and whether this makes sense at all. Whereas punq's approach keeps things much more approachable to me.

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