-
Notifications
You must be signed in to change notification settings - Fork 2.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
Patch: switch from node-ipc to ws #4185
Conversation
Looks great overall @aethernet. Iterate on this as much as you need and merge at will. |
36cbcd9
to
bab630f
Compare
1e97f88
to
15e47ed
Compare
5e2dfac
to
2d594f6
Compare
A little rough on the edge, but should be good to go for a pre-release test |
2d594f6
to
8e27a0d
Compare
8e27a0d
to
9e540b9
Compare
9e540b9
to
0a3b483
Compare
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.
Besides fixing the formatting issues which would cause unnecessary changes to be included in future PRs, my other comments aren’t critical, it’s looking fine.
0aeb805
to
f439ee1
Compare
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.
Just a few more comments focusing on clarity more than anything. Looks good otherwise.
} | ||
|
||
/** | ||
* @summary Handle `errors` |
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.
All these doc strings don’t seem to add much besides noise. Delete them? Unless you feel like there’s subtleties in each function that you need to surface as a comment, in which case by all means do so.
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 have a tendency to over-comment. It's like using your turn signal in the middle of the night, all alone on the highway; it doesn't do anything, but it's a reflex, and I prefer to do it when it's not needed rather than risk not doing it when it would be necessary.
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.
But I'm happy to remove them if you feel strongly about 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’m not offended to see them, but if optimizing for readability is a worthy goal then comments that don’t offer valuable information have no place in the code base because they contribute to noise.
5bc89db
to
67c7a6c
Compare
- switch api roles flow - use websocket instead of node-ipc - integrate; modernize; simplify and deprecate sudo-prompt
67c7a6c
to
ccc31bb
Compare
This PR does 2 things (on top )
node-ipc
tows
This makes debugging the flasher way easier and further decouple the flasher and gui, which open up the road to a standalone flasher and other cool features.