adriangb / xpresso

A composable Python ASGI web framework
https://xpresso-api.dev/
MIT License
178 stars 4 forks source link

feat/refactor!: router-level middleware, support for lifespans on mounted apps #35

Closed adriangb closed 2 years ago

adriangb commented 2 years ago

Closes #32

This is a middle road of where we are now and #32: App is not longer based on Starlette, but Router still inherits from Starlette's Router and adds middleware on top of it

codecov-commenter commented 2 years ago

Codecov Report

Merging #35 (7eca3f4) into main (5264059) will increase coverage by 0.07%. The diff coverage is 99.01%.

@@            Coverage Diff             @@
##             main      #35      +/-   ##
==========================================
+ Coverage   98.11%   98.18%   +0.07%     
==========================================
  Files         172      175       +3     
  Lines        5412     5513     +101     
  Branches      624      636      +12     
==========================================
+ Hits         5310     5413     +103     
+ Misses         56       55       -1     
+ Partials       46       45       -1     
Impacted Files Coverage Δ
...sts/test_docs/tutorial/routing/test_tutorial002.py 100.00% <ø> (ø)
xpresso/_utils/routing.py 95.65% <94.44%> (-0.90%) :arrow_down:
tests/test_routing/test_router.py 97.05% <97.05%> (ø)
tests/test_dependency_injection.py 100.00% <100.00%> (ø)
tests/test_exception_handlers.py 100.00% <100.00%> (ø)
tests/test_lifespans.py 100.00% <100.00%> (ø)
tests/test_routing/test_mounts.py 100.00% <100.00%> (ø)
xpresso/applications.py 96.77% <100.00%> (+1.23%) :arrow_up:
xpresso/exception_handlers.py 100.00% <100.00%> (ø)
xpresso/openapi/_builder.py 82.50% <100.00%> (+1.67%) :arrow_up:
... and 5 more
adriangb commented 2 years ago

@sm-Fifteen I think you may be interested in this based on your comments in encode/starlette#649(comment) and tiangolo/fastapi#617

I'm not really advertising it / putting it in the docs just yet, but this change does enable lifespans for submounted apps:

https://github.com/adriangb/xpresso/blob/3b8a438e4530cce61c2a012a6bbff9b74f0abf04/tests/test_lifespans.py#L11-L30

(incidentally, that example also uses dependency injection in the lifespan event, which I know is also something you were pushing for in the FastAPI issue)

sm-Fifteen commented 2 years ago

@adriangb: Ah, that's very interesting! I recall you mentionning moving your efforts of improving the FastAPI DI system to your own di library a few months ago, and then not really hearing much about it afterwards.

Now that I can see the vision realized as a competitor to FastAPI, and all the various improvements you made on it, namely:

...I can really say that such big changes would have been hard to pull off well on top of the existing FastAPI codebase. I'm definitely going to be following this closely!

Also, looks like you forgot to update this part: https://github.com/adriangb/xpresso/blob/b2b3ac7a122f966d0f2559e6e37f24a1945bd9c9/docs/tutorial/lifespan.md#L4

adriangb commented 2 years ago

...I can really say that such big changes would have been hard to pull off well on top of the existing FastAPI codebase. I'm definitely going to be following this closely!

Great, thanks for the kind words! I do hope that some of this eventually makes it back to FastAPI, but like you say a lot of it would be hard / impossible to re-integrate.

Also, looks like you forgot to update this part

Do you mean that we should call out support for lifespans on mounted apps? To be clear, we do not support ASGI lifespans on arbitrary mounted apps. We only support context manager lifespans on Xpresso apps. So you can't mount a Blacksheep (or other ASGI framework) app and have it run it's lifespans.

sm-Fifteen commented 2 years ago

Also, looks like you forgot to update this part

Do you mean that we should call out support for lifespans on mounted apps? To be clear, we do not support ASGI lifespans on arbitrary mounted apps. We only support context manager lifespans on Xpresso apps. So you can't mount a Blacksheep (or other ASGI framework) app and have it run it's lifespans.

Ah, right, my bad, I was skimming through the documentation and didn't realize the distinction you were trying to make. That should probably be pointed out somewhere in the "Dependency scopes" or "Lifespan" parts of the tutorial, then, since it seems like an important point to raise.

adriangb commented 2 years ago

Yup I just added that feature (in this PR) though, so I was thinking of letting it marinate a bit to see if I find any flaws in the design before really advertising it