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

reactpy.run and configure(...) refactoring #1051

Merged
merged 35 commits into from Jul 18, 2023

Conversation

Archmonger
Copy link
Contributor

@Archmonger Archmonger commented Jun 14, 2023

Issues

Right now reactpy.run can fail in fairly unpredictable ways which give little in terms of stack traces.

Summary

This refactors the reactpy.run call stack in order allow failures that are user-readable.

  • Change reactpy.backends.utils.find_all_implementations() to first try to import <backend_name> before importing reactpy.backend.<backend_name>
    • Allows for missing sub-dependencies to not cause reactpy.run to silently fail
  • Import uvicorn directly within serve_with_uvicorn in order to defer import.
    • Allows for ModuleNotFound: Could not import uvicorn exception to tell the user what went wrong
  • Added CommonOptions.serve_index_route: bool
    • Allows us to not clutter the route patterns when it's not needed
    • There are real circumstances where a user might want the index route to 404
  • Fix bug where in-use ports are being assigned on Windows.
    • Removes allow_reuse_waiting_ports parameter on find_available_port()
  • Rename BackendImplementation to BackendProtocol
  • Change load order of SUPPORTED_PACKAGES so that FastAPI has a chance to run before starlette
  • Rename SUPPORTED_PACKAGES to SUPPORTED_BACKENDS
  • Refactor reactpy.backend.* code to be more human readable
  • Use f-strings where possible
  • Merge if statements where possible
  • Use contextlib.supress where possible
  • Remove defunct requirements.txt file

Checklist

  • Tests have been included for all bug fixes or added functionality.
  • The changelog.rst has been updated with any significant changes.

@Archmonger Archmonger changed the title More predictable reactpy.run failures reactpy.run refactoring Jun 14, 2023
@Archmonger
Copy link
Contributor Author

@rmorshea npm install -g npm@8.5.0 seems to have broken in the github workflow in the last 24 hours. Not sure why, but I tried bumping actions/setup-node version. Did not fix it.

@rmorshea
Copy link
Collaborator

I was planning on doing a 1.0.1 release tomorrow. Should this wait for the next release?

@Archmonger
Copy link
Contributor Author

Yes, technically this release has an API change (BackendImplementation to BackendProtocol), so we shouldn't put this in a hotfix release.

@Archmonger Archmonger marked this pull request as ready for review June 15, 2023 06:17
@Archmonger
Copy link
Contributor Author

Archmonger commented Jun 15, 2023

I discovered that our flask backend doesn't work with uvicorn due to WsgiToAsgi not allowing non-HTTP scopes. When removing this restriction, this showed that flask-sock relies on via non-standard WSGI extensions which WsgiToAsgi does not support.

I attempted to rewrite WsgiToAsgi locally but it broke a ton of other stuff.

Ref: miguelgrinberg/flask-sock#58

@Archmonger Archmonger requested a review from rmorshea June 16, 2023 11:20
@Archmonger Archmonger changed the title reactpy.run refactoring reactpy.run and configure(...) refactoring Jun 16, 2023
Copy link
Collaborator

@rmorshea rmorshea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally just minor things to correct.

src/py/reactpy/reactpy/backend/_common.py Outdated Show resolved Hide resolved
src/py/reactpy/reactpy/backend/default.py Outdated Show resolved Hide resolved
src/py/reactpy/reactpy/backend/types.py Outdated Show resolved Hide resolved
src/py/reactpy/reactpy/backend/utils.py Outdated Show resolved Hide resolved
src/py/reactpy/reactpy/backend/utils.py Show resolved Hide resolved
@Archmonger
Copy link
Contributor Author

@rmorshea This seems to be mergeable, but needs approval.

@Archmonger Archmonger merged commit fb9c57f into reactive-python:main Jul 18, 2023
17 checks passed
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