-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Adds WebSocket support #1203
Adds WebSocket support #1203
Conversation
Signed-off-by: Mihovil Ilakovac <mihovil@ilakovac.com>
Signed-off-by: Mihovil Ilakovac <mihovil@ilakovac.com>
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.
LGTM, left some comments but overall looking great!
Will let @sodic do the final approval.
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.
Great work! Left several comments, nothing major.
@@ -13,10 +14,38 @@ export const ProfilePage = ({ | |||
}: { | |||
user: User | |||
}) => { | |||
const [messages, setMessages] = useState< | |||
{ id: string; username: string; text: string }[] |
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 would be nice if we could somehow reuse this type (the user has already defined it once).
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.
That would be nice, the user could hack it around with getting the ServerToClient events and using the Parameters
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 agreed to not doing anything fancy for now 👍
Users can define their payload types in the shared
dir and import them if they wish on the client and the server.
export interface ServerToClientEvents { | ||
chatMessage: (msg: { id: string; username: string; text: string }) => void | ||
} | ||
|
||
export interface ClientToServerEvents { | ||
chatMessage: (msg: string) => void | ||
} | ||
|
||
export interface InterServerEvents {} |
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.
Why are we exporting these types? Does making them private break full-stack type safety? I've removed the exports and everything seems to be working fine.
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.
Can we format this file (after generation) and apply the formatting here?
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.
Sure, that's a good idea 👍
// TODO(miho): This is a hack for type errors we are getting | ||
// from socket.on when passing in "handler" directly | ||
const handlerInstance: any = (event: any) => { | ||
handler(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.
Can we elaborate on the error and list a proper fix (if we know it)?
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 get the following error:
Argument of type 'ServerToClientEvents[Event]' is not assignable to parameter of type 'FallbackToUntypedListener<Event extends "disconnect" | "connect" | "connect_error" ? SocketReservedEvents[Event] : Event extends "chatMessage" ? ServerToClientEvents[Event] : never>'.
Type '(msg: { id: string; username: string; text: string; }) => void' is not assignable to type 'FallbackToUntypedListener<Event extends "disconnect" | "connect" | "connect_error" ? SocketReservedEvents[Event] : Event extends "chatMessage" ? ServerToClientEvents[Event] : never>'.
I haven't been able to figure out why that happens after digging through socket-io's source code.
And I haven't been able to find a fully working Typescript socket.io example, but I did find this workaround.
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.
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 a more detailed comment 👍
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.
How come you didn't use as keyof ServerToClientEvents
in the end?
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 forgot about that change 🤦♂️ I did it in the generated code and not in the template.
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.
Updated the code to use the cast 👍
Signed-off-by: Mihovil Ilakovac <mihovil@ilakovac.com>
Signed-off-by: Mihovil Ilakovac <mihovil@ilakovac.com>
useSocketListener('chatMessage', logMessage) | ||
|
||
function logMessage(msg: ServerToClientPayload<'chatMessage'>) { | ||
setMessages((priorMessages) => [msg, ...priorMessages]) | ||
} |
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.
Defining this function inline will allow TS to infer the type of msg
without relying on you to specify it explitily.
web/docs/typescript.md
Outdated
## WebSocket full-stack type support | ||
|
||
|
||
Defining event names with the matching payload types on the server makes those types exposed automatically on the client. This allows you avoid making mistakes when emitting events or handling them. |
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 allows you avoid making mistakes -> This helps you avoid mistakes
web/docs/typescript.md
Outdated
|
||
The `useSocketListener` hook will give you a type-safe event handler. The event name and its payload type are defined on the server. | ||
|
||
You can additonally use the `ClientToServerPayload` and `ServerToClientPayload` helper types to get the payload type for a specific event (it extract the message type from the event handler type). |
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'd remove the stuff in parentheses, they don't need to know how it does it
@@ -39,7 +33,7 @@ export interface WaspSocketData { | |||
} | |||
|
|||
type WebSocketFn = typeof {= userWebSocketFn.importIdentifier =} | |||
type ServerType = Parameters<WebSocketFn>[0] | |||
export type ServerType = Parameters<WebSocketFn>[0] |
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.
Can we put all exports at the top?
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.
Feels a little strange to have a folder called webSocket
and a file called webSocket.ts
at the same level (we're not in Haskell, after all :). Especially because that folder only contains a single file.
Maybe we can call this one webSocket/index.ts
or something?
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.
Makes sense 👍
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.
Maybe initalization.ts
is a better name than server.ts
? Not sure.
Signed-off-by: Mihovil Ilakovac <mihovil@ilakovac.com>
Description
Adds support for WebSockets (via Socket.IO), adds a
useSocket
abstraction, and demonstrates a use of shared types on the client and server.Example from docs (https://github.com/wasp-lang/wasp/blob/e4745a7bcc20ffe3a90b48a290cca949603665d0/web/docs/guides/websockets.md) integrated into example app. You can chat on profile page. We can remove if not helpful.
TODO:
Select what type of change this PR introduces:
Update Waspc ChangeLog and version if needed
If you did a bug fix, new feature, or breaking change, that affects waspc, make sure you satisfy the following: