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

Rework How IDOM Integrates With Servers #703

Merged
merged 58 commits into from
Mar 28, 2022
Merged

Rework How IDOM Integrates With Servers #703

merged 58 commits into from
Mar 28, 2022

Conversation

rmorshea
Copy link
Collaborator

@rmorshea rmorshea commented Mar 6, 2022

closes: #657

The change proposed here is that IDOM will expose a top-level idom.develop function that takes in a component definition and not much else. The configuration options here are intentionally limited since we want to push people away from using this function in production.

This change will also remove PerClientStateServer and SharedClientStateServer with a single configure function for each server implementation:

from idom.server.sanic import configure
configure(app, MyComponent)

As part of this change SharedClientStateServer will be removed. While it's cool, it can't be used in a production context - you need a real cross-process solution for synchronizing state (e.g. Redis).

Other issues I plan to address here:

closes: #678, closes #591, closes #669, closes #657

@rmorshea rmorshea force-pushed the develop branch 3 times, most recently from 5a998b6 to dcdda61 Compare March 6, 2022 07:07
@rmorshea rmorshea changed the title initial work to move away from run func Rework How IDOM Apps Are Run Mar 7, 2022
@rmorshea rmorshea changed the title Rework How IDOM Apps Are Run Rework How IDOM Integrates With Servers Mar 7, 2022
@Archmonger
Copy link
Contributor

Archmonger commented Mar 10, 2022

Will need to keep in mind that readme and docs modifications will need to occur here too.

I don't know if I agree with adding an app parameter to idom.run since it uglifies how new users will test IDOM. I think a warning message along with idom.run only taking one parameter (component) will suffice.

@rmorshea rmorshea mentioned this pull request Mar 21, 2022
@Archmonger
Copy link
Contributor

Here's an alternative syntax to consider, based more identically on WhiteNoise's design

from sanic import Sanic
from idom.server.sanic import IdomSanic

# Configure returns a wrapped ASGI/WSGI callable
asgi_app = IdomSanic(Sanic("My App"), MyComponent, endpoint="/idom")

@rmorshea
Copy link
Collaborator Author

I don't think IDOM has a particularly compelling reason to create provide an ASGI app, so for now I think sticking with a simple configure function should be fine.

@Archmonger
Copy link
Contributor

Archmonger commented Mar 22, 2022

The only compelling reason is if you want IDOM to behave more like configurable WSGI/ASGI middleware. Fairly common design pattern in the Python web dev space.

Side note, I'm assuming the reason the configure function exists is to set up the WS/HTTP endpoints, as mentioned in #653 ?

@rmorshea
Copy link
Collaborator Author

rmorshea commented Mar 22, 2022

By "compelling reason", I mean that I can't think of any methods or middleware to add to an IDOM ASGI app.

And yes, the recommended way of running IDOM would be to instantiate a support application, idom.server.*.configure() it, and then run the app with an ASGI server.

@Archmonger
Copy link
Contributor

Not pushing for this change, both design patterns accomplish the same task. But on the topic of middleware, it doesn't necessarily need to add methods. See Whitenoise().__call__.

@rmorshea
Copy link
Collaborator Author

I'm just not sure what IDOM would want to do in the __call__ method of its ASGI app.

I think we can stick with the configure method for now. If it turns out we want to create an ASGI wrapper it should be pretty straightforward to do so. We might not even need to deprecate the configure function since the ASGI app might continue to call that.

@rmorshea rmorshea force-pushed the develop branch 2 times, most recently from 2969a4c to 425c531 Compare March 26, 2022 06:39
@rmorshea rmorshea marked this pull request as ready for review March 26, 2022 07:49
@rmorshea rmorshea force-pushed the develop branch 6 times, most recently from 2388ee5 to a4c0d84 Compare March 27, 2022 23:21
@rmorshea rmorshea merged commit f2c1070 into main Mar 28, 2022
@rmorshea rmorshea deleted the develop branch March 28, 2022 00:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants