-
Notifications
You must be signed in to change notification settings - Fork 30k
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
named anonymous functions in readline & zlib.js #21792
Conversation
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.
Hi @antsmartian, the goal of naming these functions should be to make a more useful name than the anonymous one. Please try to consider the use cases for these functions and name based on that. In particular this applies to fnWrap
.
Yeah sure. Will do the same and update. |
@apapirovski Ok, I did skimmed the source code of For |
31d0635
to
6e0e792
Compare
@@ -843,7 +843,7 @@ Interface.prototype._ttyWrite = function(s, key) { | |||
if (this.listenerCount('SIGTSTP') > 0) { | |||
this.emit('SIGTSTP'); | |||
} else { | |||
process.once('SIGCONT', (function(self) { | |||
process.once('SIGCONT', (function continueProcess(self) { |
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.
So we could actually do a slightly better thing here. Instead of the IIFE, we could use an arrow function and replace self
with this
— unless I'm missing something. Then we will just have a normal event callback instead of this weird thing it is right now. Feel free to either do that change here or in a separate PR.
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.
Yes that make sense, may be will update the same in another PR. Thanks for your time on this.
Thank you @antsmartian for your first PR in Node.js core! The |
6e0e792
to
d8ec3d9
Compare
@trivikr Thanks, for some reason, my git config wasn't correct. Now fixed. |
Resumed once again, infrastructure issue on OSX: https://ci.nodejs.org/job/node-test-pull-request/16141/ |
Landed in fc6f49a |
PR-URL: #21792 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me>
PR-URL: #21792 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes