-
Notifications
You must be signed in to change notification settings - Fork 0
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
チャットルーム作成機能を実装した #29
チャットルーム作成機能を実装した #29
Conversation
確認しました。 別のマージによりconflictがあるようなので、その対応後にも確認しますが、front/backendを起動させたときの報告です。 backend: frontend: shogo@[0:29:27]:~/work/development/42Tokyo/ft_transcnendence/add-create-chat-room/frontend% yarn && yarn build && yarn start Failed to compile. ./pages/chat/index.tsx info - Need to disable some ESLint rules? Learn more here: https://nextjs.org/docs/basic-features/eslint#disabling-rules |
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.
動作に関してはきちんと確認できました!ありがとうございます。
|
||
@WebSocketServer() server: Server; | ||
|
||
private logger: Logger = new Logger('ChatGateway'); |
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.
これbackend全体で統一して使ったほうが見栄えよさそうですね
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.
確かに全体で統一してもいいかもですね。
でもnewするときに名前を指定できるから、モジュールごととかでもいいのかな。
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.
導入するかどうかを調査するissue作っときます
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.
ドキュメントで各サービスごとにインスタンス化することを推奨していたので、今の感じでいきましょう
#43
}); | ||
|
||
return () => { | ||
socket.off('room:created'); |
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.
実はあります。
開発環境だとstrict modeが効いているので、複数回レンダリングされます。
useEffectにsocket.onを書いても、レンダリングされた回数イベントハンドラーが適用されるので、
offがないと、本来1回しか呼ばれてほしくないイベントが複数呼ばれることになってしまいます。
具体的な例をあげると、チャットルームを作成するたび、同じものが2回listに追加されるという挙動になるはずです。
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.
参考
the listeners must be removed in the cleanup step, in order to prevent multiple event registrations
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.
なるほど、理解しました。ありがとうございます!!
リポジトリを作成し直して、手順通りで、Chatroomの作成が確認できました。 |
概要
関連issue
その他
テスト方法
demo
動画でprismaに登録されたidが13, 14なのは事前にテストしてたから
2022-10-25.16.45.47.mov