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

refactor!: rework app router relationship #32

Closed
wants to merge 7 commits into from

Conversation

adriangb
Copy link
Owner

@adriangb adriangb commented Jan 30, 2022

This change:

  • Moves the responsibility of middleware to Router, effectively enabling Router-level middleware that will work with Mount, Host, etc.
  • Moves the responsibility of lifespans into App since there can only ever be 1 lifespan and there can only be 1 App.

As part of this, App no longer inherits from Starlette and Router no longer inherits from its counterpart in Starlette.
This both simplifies things and complicates them a bit, but it does give us the opportunity to do a bit of cleanup and performance optimization (slots, removing lifespan wrappers and making routing sans-io)

There are obviously cons to this though. I might let this stew a bit and see.

Here are the benchmark numbers using the benchmarks in /benchmark:

/simple /fast_deps /slow_deps
main 2.19 1.44 0.16
this 2.40 1.38 0.16
starlette 4.47
fastapi 2.76 0.63 .08

So it looks like this is a pretty solid +10% performance boost for simple scenarios

@codecov-commenter
Copy link

codecov-commenter commented Jan 30, 2022

Codecov Report

Merging #32 (378df0f) into main (7985030) will decrease coverage by 0.49%.
The diff coverage is 73.21%.

@@            Coverage Diff             @@
##             main      #32      +/-   ##
==========================================
- Coverage   98.11%   97.62%   -0.50%     
==========================================
  Files         172      172              
  Lines        5412     5473      +61     
  Branches      624      639      +15     
==========================================
+ Hits         5310     5343      +33     
- Misses         56       78      +22     
- Partials       46       52       +6     
Impacted Files Coverage Δ
xpresso/routing/router.py 66.12% <63.79%> (-33.88%) ⬇️
xpresso/applications.py 90.24% <80.43%> (-5.30%) ⬇️
xpresso/_utils/routing.py 96.66% <100.00%> (+0.11%) ⬆️
xpresso/exception_handlers.py 100.00% <100.00%> (ø)
xpresso/responses.py 100.00% <100.00%> (ø)

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

2 participants