-
-
Notifications
You must be signed in to change notification settings - Fork 943
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
Remove on_startup, on_shutdown, event system and deprecated lifespans #2068
Conversation
what do you think about adding a little "migration" guide like: you used to do this:
now do this:
or it's too obvious ? |
I don't think it's too obvious, I agree we should add it. But we should probably do the same for all of the things we are deprecating / removing. Maybe it's time to start a |
await database.connect() | ||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't those 2 lines be interverted ?
|
||
A single lifespan `asynccontextmanager` handler can be used instead of | ||
separate startup and shutdown handlers: | ||
Starlette will not start serving any incoming requests until the lifespan has been run. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean uvicorn right ?
The lifespan can also accept a `state` argument, which is a dictionary | ||
that can be used to share the objects between the startup and shutdown handlers, | ||
and the requests. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems to me that this becomes outdated since there wont be startup and shutdown handlers
So, what do we do now? This is a breaking change.
This is really not obvious. Is there any way to get the equivalent functionality of on_startup, on_shutdown more intuitively? The DX of being able to just specify a function for these important events is a good thing. |
Side "meta" note, what is with all the breaking PR's this year? Is the plan to force everyone to completely re-learn Starlette and re-write their apps for 1.0... 🤷 What makes this more egregious is the proposed patterns have worse DX |
Your example is very contrived, I seriously doubt anyone is doing that. |
Not my code, re: #2068 (comment) |
Misread. Thought you were yielding a something, which, indeed, would be breaking, but like I said very contrived. We have documentation for this. And there are good reasons why this approach is much better than two separate functions. I’m afraid I do not agree that it is very non-obvious how to use. |
Is this what you're referring to? |
@@ -3,6 +3,7 @@ Starlette includes an application class `Starlette` that nicely ties together al | |||
its other functionality. | |||
|
|||
```python | |||
from contextlib import asyccontextmanager |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
asyccontextmanager
should be asynccontextmanager
@@ -25,8 +26,10 @@ async def websocket_endpoint(websocket): | |||
await websocket.send_text('Hello, websocket!') | |||
await websocket.close() | |||
|
|||
def startup(): | |||
@asyccontextmanager |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
asyccontextmanager
should be asynccontextmanager
Decisions can be reversed until 1.0... But a better place to talk about it would be the discussion that was taken. I'm on my phone right now. |
The idea of deprecating things now is to avoid doing so on 1.0. Feedback is always welcome, and if we made a mistake, we can revert decisions. |
For people reading this PR, FastAPI went official with this in 0.93.0, only a few days ago: #2067 (comment) Totally fair with the discussion that happened in: #2067 That's fine. The cat is out of the bag at this point. Gonna go make the changes to my Starlette apps now. |
Cool. But in any case, we are always open to take decisions back if they make sense. We don't want to harm users, and I think we try to always be transparent here. |
@Kludex |
Ref.: mypyc/mypyc#868 |
You don't need to use async generators to create an async context manager you can just manually implement |
#2067
Close to 500 LOC removed. That's ~ 1/20th of the codebase.