lib: name anonymous callbacks by mlrv · Pull Request #21412 · nodejs/node · GitHub

First contribution here!

This commit is to help in the effort to name all anonymous
functions to help when heap debugging. Specifically, this commit
fixes some anonymous functions used as listeners in the lib/ folder.

Refs: #8913

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Is that supposed to be replaceCallback instead of replacerCallback?

It wasn't a typo in my head. I suppose that it makes sense in both versions:

replacerCallback -> the replacer function (inside replace)
replaceCallback -> the callback of the replace function

Happy to change it of course

Nah, if it wasn't a typo, it's fine with me. Just wanted to give you the opportunity to fix it up if it was a mistake.

LGTM if CI is green

CI: https://ci.nodejs.org/job/node-test-pull-request/15529/

Thanks for doing this and following up! 🎉

Glad to see it all seems to go well :)

If I understood correctly, the standard process is to wait at least 48 hours before merging, am I right?

Is there anything else that needs to be done?

@mlrv that is correct, we can fast track this one though since it's naming rather than functionality. This is all explained here: https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#waiting-for-approvals . It also explicitly states that since this is a good-first-issue then fast-tracking might be appropriate.

Collaborators 👍 this comment to support fast-tracking this change.

CI again to try to confirm Windows failure is unlikely to be related: https://ci.nodejs.org/job/node-test-pull-request/15538/

Hmmm... let's try a total re-run:

CI: https://ci.nodejs.org/job/node-test-pull-request/15555/

Hi @mlrv — thank you for working on this. In my opinion naming should provide some value, rather than just being decorative. Names such as socketOnceCallback do not help anyone and do not make the stack trace any more descriptive than an anonymous function would. I've left some comments regarding some potentially more useful names.

pipeOnDrainCallback? It won't be "inner" by the time it's in a stack trace.

socketSetTimeoutOnConnect?

socketDestroyOnConnect?

Switch to an anonymous function instead of naming it? The function name is not particularly useful.

I thought there was consensus that naming anonymous callback functions was helpful. If that's not the case, I should update #8913 (comment).

@apapirovski any update on this?

My bad here as far as dropping the ball and misphrasing. What I meant to say is: I think this should just be an arrow function. I don't think we gain much by naming callbacks for things like String.prototype.replace.

I implemented the suggestions and fixed the conflicts. Happy to go ahead if all the checks are green?

Hi @apapirovski, thanks for the feedback, I totally see your point. Happy to change the names if you think those could be more helpful. On the last comment, not sure I agree with you, what do other people think?

I would suggest just using (all, key, val) => { here.

@benjamingr all green, good to go? :)

CI: https://ci.nodejs.org/job/node-test-pull-request/16104/

not sure about the arrow function fitting in with the rest but won't block

Weird. CI failed on git due to a conflict. Let's try again...

CI: https://ci.nodejs.org/job/node-test-pull-request/16105/

GitHub says this has no conflicts with the base branch, but manually trying to apply the changes shows conflicts. I'll resolve them, push back up, and restart CI.

This commit is to help in the effort to name all anonymous functions to help when heap debugging. Specifically, this commit fixes some anonymous functions used as listeners in the lib/ folder. Refs: nodejs#8913

CI: https://ci.nodejs.org/job/node-test-pull-request/16107/

Might be good for people to double-check the changes now that I've done a rebase and resolved conflicts.

Resume Build: https://ci.nodejs.org/job/node-test-pull-request/16165/

This commit is to help in the effort to name all anonymous functions to help when heap debugging. Specifically, this commit fixes some anonymous functions used as listeners in the lib/ folder. PR-URL: #21412 Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Refs: #8913

Landed in d7496bf, congrats on your first PR to Node.js!
❤️ 💚 💙 💛 💜

This commit is to help in the effort to name all anonymous functions to help when heap debugging. Specifically, this commit fixes some anonymous functions used as listeners in the lib/ folder. PR-URL: #21412 Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Refs: #8913

nodejs-github-bot added http Issues or PRs related to the http subsystem. stream Issues and PRs related to the stream subsystem. tls Issues and PRs related to the tls subsystem. labels Jun 19, 2018

Trott reviewed Jun 19, 2018

View reviewed changes

Trott previously approved these changes Jun 19, 2018

View reviewed changes

starkwang approved these changes Jun 20, 2018

View reviewed changes

trivikr approved these changes Jun 20, 2018

View reviewed changes

trivikr added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jun 20, 2018

benjamingr approved these changes Jun 20, 2018

View reviewed changes

BridgeAR approved these changes Jun 20, 2018

View reviewed changes

BridgeAR added the fast-track PRs that do not need to wait for 48 hours to land. label Jun 20, 2018

lpinca approved these changes Jun 20, 2018

View reviewed changes

jasnell approved these changes Jun 21, 2018

View reviewed changes

apapirovski suggested changes Jun 25, 2018

View reviewed changes

addaleax removed author ready PRs that have at least one approval, no outstanding review comments, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. labels Jul 13, 2018

maclover7 added the wip Issues and PRs that are still a work in progress. label Jul 28, 2018

apapirovski reviewed Jul 30, 2018

View reviewed changes

apapirovski approved these changes Jul 30, 2018

View reviewed changes

benjamingr added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jul 30, 2018

mlrv added 4 commits July 31, 2018 18:37

lib: name anonymous callbacks

607ee37

This commit is to help in the effort to name all anonymous functions to help when heap debugging. Specifically, this commit fixes some anonymous functions used as listeners in the lib/ folder. Refs: nodejs#8913

Improved naming

72c2573

Make linter happy

fa194ca

Use arrow function

b1e3dc1

Trott force-pushed the marco/issue-8913 branch from 760ec4c to b1e3dc1 Compare August 1, 2018 01:39

maclover7 approved these changes Aug 3, 2018

View reviewed changes

maclover7 removed the wip Issues and PRs that are still a work in progress. label Aug 3, 2018

maclover7 closed this Aug 4, 2018

rvagg mentioned this pull request Aug 13, 2018

Release proposal: v10.9.0 #22295

Merged

lib: name anonymous callbacks by mlrv · Pull Request #21412 · nodejs/node · GitHub

Uh oh!

Uh oh!

lib: name anonymous callbacks - #21412

lib: name anonymous callbacks#21412mlrv wants to merge 4 commits intonodejs:masternodejs/node:masterfrom mlrv:marco/issue-8913mlrv/node:marco/issue-8913Copy head branch name to clipboard

Conversation

mlrv commented Jun 19, 2018

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlrv Jun 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

trivikr commented Jun 20, 2018

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

mlrv commented Jun 20, 2018

Uh oh!

benjamingr commented Jun 20, 2018

Uh oh!

Trott commented Jun 20, 2018

Uh oh!

Trott commented Jun 21, 2018

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlrv commented Jun 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlrv commented Jul 30, 2018

Uh oh!

Trott commented Aug 1, 2018

Uh oh!

Trott commented Aug 1, 2018

Uh oh!

Trott commented Aug 1, 2018

Uh oh!

Trott commented Aug 1, 2018

Uh oh!

Trott commented Aug 1, 2018

Uh oh!

Trott commented Aug 3, 2018

Uh oh!

maclover7 commented Aug 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

lib: name anonymous callbacks#21412
mlrv wants to merge 4 commits into
nodejs:masterfrom
mlrv:marco/issue-8913

mlrv Jun 20, 2018 •
edited
Loading