-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Use Socket.IO for message transport #449
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome job!! Tested locally and it worked nicely - just a couple comments
@@ -85,24 +100,6 @@ def __call__(self) -> FastAPI: | |||
""" | |||
return self.api | |||
|
|||
def add_default_endpoints(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should keep the ping
endpoint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added it back in. For future reference it can be tested like so:
socket.current.emit('ping')
console.log('ping')
socket.current.on('ping', function(data) {
console.log(data);
});
In the console you should see:
ping
pong
# To make state changes. | ||
self.api.websocket(str(constants.Endpoint.EVENT))(event(app=self)) | ||
|
||
def add_cors(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we not need this anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's taken care of when the AsyncServer
is created on the App
__init__
:
self.sio = AsyncServer(async_mode="asgi", cors_allowed_origins="*")
There's also cors_credentials=
which defaults to True
already.
python-scoketio does not have CORS options like allow_methods
and allow_headers
but I believe it's configured to avoid cross-origin problems with browsers by allowing all headers and methods anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay got it nice
@@ -327,52 +324,6 @@ def compile(self, force_compile: bool = False): | |||
compiler.compile_components(custom_components) | |||
|
|||
|
|||
async def ping() -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be useful for health checks, etc.
pynecone/app.py
Outdated
self.sio = AsyncServer(async_mode="asgi", cors_allowed_origins="*") | ||
|
||
# Create the socket app. Note 'event' replaces the default 'socket.io' path. | ||
socket_app = ASGIApp(self.sio, socketio_path="event") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use constants.Endpoint.EVENT instead of a string here (and in the EventNameSpace below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do that. Note that str(constants.Endpoint.EVENT)
returns "/event"
not "event"
so I'll also update state.js accordingly, since those have to match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually that might cause a bunch of confusion in the future, for example you call ping with 'ping'
but it returns on '\ping'
. So I'll modify constants.py `Endpoint' to allow it to return the literal value without the leading ''.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I think about it it's still confusing. There's potential confusion between the URL endpoint /event
(a namespace) and the socket event event
(they're separate concepts in socket.io). I'll separate those in constants.py to avoid future problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
Also, can you update the |
Thanks for the feedback. Please see the latest commits and let me know if there's anything else! |
Magnifique, you're the boss @advo-kat You rock ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet great job!
# To make state changes. | ||
self.api.websocket(str(constants.Endpoint.EVENT))(event(app=self)) | ||
|
||
def add_cors(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay got it nice
All Submissions:
Type of change
Please delete options that are not relevant.
New Feature Submission:
Description
A rework of the websocket implementation in Pynecone.
Benefits
AsyncServer
, it will be easier to implement implement server side / async events in the future/socket.io
endpoint has been replaced with the usual/event
endpointI've tested this PR locally and on a production server.