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

add use_connection hook #823

Merged
merged 8 commits into from Oct 30, 2022
Merged

add use_connection hook #823

merged 8 commits into from Oct 30, 2022

Conversation

rmorshea
Copy link
Collaborator

@rmorshea rmorshea commented Oct 19, 2022

closes: microsoft/playwright-python#819

Rather than having a hook registration framework. All we need in order to solve the problem at hand is to make it possible for each respective backend to use a common context through which they can declare a scope and location. To do this we define a ConnectionContext that each backend must activate with a Connection object. The Connection object allows the backend to declare scope, location, and an additional, implementation specific, carrier. The carrier is intended to describe the manner in which a connection is mediated (e.g. an HTTP request or websocket).

As a result of these changes, there are now common hooks for the following:

  • use_connection() -> Connection[Any]
  • use_scope() -> dict[str, Any]
  • use_location() -> Location

Backend implementations provide their own idom.backend.*.use_connection hooks, but these return the exact same object as the common idom.use_connection hook. The only difference is that the type annotation for the return type is narrowed to reflect the carrier of the backend. In narrowing the type though, various backend implementation are also provide implementation specific use_request or use_websocket hooks.

Checklist

Please update this checklist as you complete each item:

  • Tests have been included for all bug fixes or added functionality.
  • The changelog.rst has been updated with any significant changes.
  • GitHub Issues which may be closed by this Pull Request have been linked.

@rmorshea rmorshea force-pushed the use_connection branch 2 times, most recently from 7d2617b to fc3f28c Compare October 19, 2022 19:58
@rmorshea
Copy link
Collaborator Author

@Archmonger to make this work with django-idom the approach would be remove the custom WebsocketContext in favor of IDOM core's ConnectionContext. See the starlette implementation for an example.

@rmorshea rmorshea marked this pull request as ready for review October 19, 2022 20:53
@rmorshea
Copy link
Collaborator Author

@Archmonger does this look good to you?

@Archmonger
Copy link
Contributor

Been pretty busy these last two weeks, haven't had time to do a code review. Hopefully will have time at night during the upcoming weekdays.

Copy link
Contributor

@Archmonger Archmonger left a comment

Choose a reason for hiding this comment

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

I think we shouldn't go all-in on changing websocket to carrier.

The more I think about our reliance on an asyncio running loop, the more impossible it seems for us to use anything other than Websockets.

HTTP simply doesn't have any decent level of persistence, and it's awkward/unreasonable to make every user run an "IDOM server" separately as a backhaul running loop. I've been debating closing that issue related to this.

Everything else LGTM.

@rmorshea
Copy link
Collaborator Author

rmorshea commented Oct 29, 2022

I don't really see any harm in using a generic name like carrier. Connections could be mediated by websockets, regular sockets, a multiprocessing pipe, or even some in-memory construct depending on the use case. For example, I could see a pipe or in-memory construct being used if we ever developed some integration with a terminal interface like textual. If you actually need access to the carrier you probably know the type of carrier you're dealing with, in which case you're going to use a backend specific hook (e.g. use_websocket) anyway.

tests/test_widgets.py Outdated Show resolved Hide resolved
@rmorshea
Copy link
Collaborator Author

I'd like to merge this so I can get started integrating these changes into idom-router. I don't think there's anything that will be impacted if we decide to rename the carrier attribute so we can discuss that possibility in an issue if needed.

@rmorshea rmorshea merged commit 64c9e82 into main Oct 30, 2022
@rmorshea rmorshea deleted the use_connection branch October 30, 2022 19:12
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