Remove on_startup, on_shutdown, event system and deprecated lifespans#2068
Remove on_startup, on_shutdown, event system and deprecated lifespans#2068adriangb wants to merge 4 commits intoKludex:masterfrom
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.
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. |
| 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.
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? |
| its other functionality. | ||
|
|
||
| ```python | ||
| from contextlib import asyccontextmanager |
There was a problem hiding this comment.
asyccontextmanager should be asynccontextmanager
| await websocket.close() | ||
|
|
||
| def startup(): | ||
| @asyccontextmanager |
There was a problem hiding this comment.
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.