chore: convert some server files into TypeScript #757
Conversation
Naturalclar
requested review from
Esemesek,
grabbou and
thymikee
as code owners
| import {logger} from '@react-native-community/cli-tools'; | ||
| import {Server as HttpServer} from 'http'; | ||
| import {Server as HttpsServer} from 'https'; | ||
| import WebSocket from 'ws'; |
There was a problem hiding this comment.
ws is imported twice, can we do it one?
|
cc @Esemesek |
|
Thanks for the feedback! I've made some changes to improve with typings and removed some |
grabbou
left a comment
grabbou
left a comment
There was a problem hiding this comment.
Happy to see another PR towards full TypeScript migration! Left some feedback as well.
|
|
||
| function launchChrome(url: string) { | ||
| return open(url, {app: [getChromeAppName()], wait: true}); | ||
| async function launchChrome(url: string) { |
There was a problem hiding this comment.
Please revert this whole change. It introduces a regression, since we don't want to catch this exception here
| | 'emacsclient' | ||
| | 'rmate' | ||
| | 'mate' | ||
| | 'mine'; |
There was a problem hiding this comment.
Nah, we don't have resources to maintain this list. Please use just string
| clientSocket.onerror = clientSocketCloseHandler; | ||
| clientSocket.onclose = clientSocketCloseHandler; | ||
| clientSocket.onmessage = ({data}) => send(debuggerSocket, data); | ||
| if (clientSocket) { |
There was a problem hiding this comment.
we assume connection is defined on line 52, let's skip this check if possible
There was a problem hiding this comment.
changed the type of connection in this function argument and removed this check
| clientSocket.onmessage = null; | ||
| clientSocket.onerror = () => {}; | ||
| clientSocket.onclose = () => {}; | ||
| clientSocket.onmessage = () => {}; |
There was a problem hiding this comment.
I don't think these reassignments are necessary, because they're overwritten right after. It only makes sense to null them when clientSocket.close throws for some reason and we try to recover – the handlers could leak memory because clientSocket would hold them. I'm not sure if that happens, didn't have time to go through this deeply.
For now, let's revert to what was here before and ts-ignore it with a todo comment to rewrite, once we migrate to newer version of ws library.
thymikee
changed the title
|
Thank you! |
3 participants
Summary:
This PR is part of #683
Converted some of server related files into TypeScript
Test Plan: