feat(runtime,serverless,cli): add WebSocket support #887
Conversation
🦋 Changeset detectedLatest commit: 878d496 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@akitaSummer is attempting to deploy a commit to the Lagon Team on Vercel. A member of the Team first needs to authorize it. |
akitaSummer
force-pushed
the
feat/websocket
branch
from
edbe0a2
to
38262e7
Compare
akitaSummer
force-pushed
the
feat/websocket
branch
from
38262e7
to
d8b1789
Compare
|
This pr did not take into account the link reuse of websockets in distributed scenarios. I think there is still more optimization. |
|
Thanks a lot for this PR! I'll take a deep look at it tomorrow. We'll probably need to add some limits (e.g max number of open websockets, max message size, ...) |
akitaSummer
force-pushed
the
feat/websocket
branch
from
d8b1789
to
9a02be6
Compare
QuiiBz
changed the title
crates/runtime/tests/websocket.rs
Outdated
| ws.send('test_ws'); | ||
| ws.close(); | ||
| // TODO: If you add this line, it will always block, but the correct logic is that if you don’t add this line, it will always block. I don’t understand why | ||
| // res(); |
There was a problem hiding this comment.
Shouldn't we call res() on the close event?
There was a problem hiding this comment.
What I mean is that the current implementation should theoretically block the code at the await statement, but in practice, it continues execution until the end.
There was a problem hiding this comment.
If the comment on line 55 is uncommented, the code will block at the await statement. However, the correct logic should be to continue execution until the end.
There was a problem hiding this comment.
This is exactly why I suggested #887 (comment): JS-land shouldn't manage or create a parallel event loop, it should be done in native-land. JS-land doesn't have access to the whole context, so it's almost impossible to make it work this way.
There was a problem hiding this comment.
I didn't understand the connection between JS-land and the bug in the current code. I think after line 54 is executed, the code should block at the await because res() is not called. Is this related to Native-land? Did Native-land resolve this promise?
| private async eventLoop() { | ||
| while (this._readyState !== WsState.CLOSED) { | ||
| try { | ||
| const data = await LagonAsync.websocketEvent(this.wsId); |
There was a problem hiding this comment.
I think this part should be moved inside the runtime's event loop directly (runtime_isolate/src/lib.rs), where we can call ws.next_message()
There was a problem hiding this comment.
I don't agree with that. I believe this code involves modifying the state of the ws object in JavaScript and involves significant data interaction, such as creating an Event object. Therefore, I suggest performing the event loop on the JavaScript side.
There was a problem hiding this comment.
See my answer here: #887 (comment)
There was a problem hiding this comment.
I reused the event loop in JS-land here, and I think it's not correct to handle events on the Native-land side. Firstly, as you can see from the code I wrote, I don't need much context information. I only need Native-land to tell me the current return value of ws, and then I trigger the corresponding subscriptions based on the return value. If I handle events on the Native-land side, the required content will not change, and triggering subscriptions can change the status of ws in JS-land, which can bring more costs. Maybe I have misunderstood your needs. If you have better ideas, can you directly modify this branch so that we can discuss further?
There was a problem hiding this comment.
I'll push to this PR directly so you can better understand what I mean, and then we can discuss which solution is the best.
|
@akitaSummer I've rebased and updated the js-runtime code to fix the TypeScript errors and implement both |
akitaSummer
force-pushed
the
feat/websocket
branch
from
50be078
to
e23fde0
Compare
Now you can use websocket.
But the Event is incorrect, I temporarily used @ts-ignore to hack, I hope you can solve this problem in the future, and we still lack CloseEvent and ErrorEvent.
#794