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/refactor!: router-level middleware, support for lifespans on mounted apps #35

Merged
merged 5 commits into from
Feb 1, 2022

Conversation

adriangb
Copy link
Owner

@adriangb adriangb commented Feb 1, 2022

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

@adriangb adriangb changed the title test lifespan and exception handlers feat/refactor!: router-level middleware, support for lifespans on mounted apps Feb 1, 2022
@codecov-commenter
Copy link

codecov-commenter commented Feb 1, 2022

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%) ⬇️
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%) ⬆️
xpresso/exception_handlers.py 100.00% <100.00%> (ø)
xpresso/openapi/_builder.py 82.50% <100.00%> (+1.67%) ⬆️
... and 5 more

@adriangb
Copy link
Owner Author

adriangb commented Feb 1, 2022

@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:

def test_lifespan_mounted_app() -> None:
class Counter(List[int]):
pass
@asynccontextmanager
async def lifespan(counter: Counter) -> AsyncIterator[None]:
counter.append(1)
yield
app = App(
routes=[Mount("/mounted-app", app=App(lifespan=lifespan))], lifespan=lifespan
)
counter = Counter()
app.container.register_by_type(Dependant(lambda: counter, scope="app"), Counter)
with TestClient(app):
pass
assert counter == [1, 1]

(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)

@adriangb adriangb merged commit 4bcc415 into main Feb 1, 2022
@adriangb adriangb deleted the app-lifespan-router-middleware-redux branch February 1, 2022 06:30
@sm-Fifteen
Copy link

sm-Fifteen commented Feb 2, 2022

@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:

This is the only way to handle startup/shutdown; there are no startup/shutdown events in Xpresso.

@adriangb
Copy link
Owner Author

adriangb commented Feb 2, 2022

...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
Copy link

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
Copy link
Owner Author

adriangb commented Feb 2, 2022

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

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