Async hooks rename#14152
Async hooks rename #14152
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added the async_hooks 
There was a problem hiding this comment.
This change wasn't needed. But when I looked for the AsyncResource('default_trigger_id') test, I found that it could be made more strict.
lib/async_hooks.js Outdated
There was a problem hiding this comment.
So just an FYI since this effectively changes the signature of the ctor (now AsyncResource.length == 1it would have been some do consider it semver-major, but since this congruent with the docs, I'd call this a bug fix.
There was a problem hiding this comment.
Also this is still officially experimental, which should give us more flexibility.
There was a problem hiding this comment.
Could you add assert.strinctEqual(AsyncResource.length, 1);
Retracted
I’m strongly against considering changes semver-major because they modify the value of (edit: and, by extension, we should not need to test the value of |
refack commented Jul 10, 2017
I agree, but some do consider it as such 🤷♂️ Should bring it up with the release team or the CTC. |
AndreasMadsen commented Jul 12, 2017
AndreasMadsen commented Jul 12, 2017
There were a huge amount of unrelated CI failures. It looks like most of it have been fixed in master now. I have rebased on master, hopefully that will fix it.
|
There was a problem hiding this comment.
nit: this looks a little odd syntactically. maybe something like:
constasync_hooks=require('async_hooks');const{ AsyncResource }=async_hooks;
trevnorris left a comment
trevnorris left a comment There was a problem hiding this comment.
Have a non-blocking nit. Please don't forget to squash the two commits, and maybe add a message body with more explanation.
Really, you want the two commits squashed? Logically they are very completely separate. |
@AndreasMadsen No. It's a friendly reminder to make sure the commit
is squashed into whatever commit it belongs. It's also not necessarily a reminder for you, but whomever it is that lands the PR. |
AndreasMadsen commented Jul 13, 2017
I fixed the nit and added some more text in the commit message. If the CI is green I will just go ahead and merge. |
There are two categories of emit functions in async_hooks, those used by c++ (native) and those used by JavaScript (script). Previously these were named N for native and S for script. Finally, there was an odd case where emitInitN was called just init. This makes it more explicit by using the names emitInitNative and emitInitScript. The other emit functions are also renamed.
AsyncResource previously called emitInitNative. Since AsyncResource is just an abstraction on top of the emitEventScript functions, it should call emitInitScript instead.
AndreasMadsen commented Jul 13, 2017
Fixed a small merge conflict. |
AndreasMadsen commented Jul 13, 2017
trevnorris commented Jul 13, 2017
@AndreasMadsen Thanks for the two thorough git commit messages. |
There are two categories of emit functions in async_hooks, those used by c++ (native) and those used by JavaScript (script). Previously these were named N for native and S for script. Finally, there was an odd case where emitInitN was called just init. This makes it more explicit by using the names emitInitNative and emitInitScript. The other emit functions are also renamed. PR-URL: #14152 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
AsyncResource previously called emitInitNative. Since AsyncResource is just an abstraction on top of the emitEventScript functions, it should call emitInitScript instead. PR-URL: #14152 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
There are two categories of emit functions in async_hooks, those used by c++ (native) and those used by JavaScript (script). Previously these were named N for native and S for script. Finally, there was an odd case where emitInitN was called just init. This makes it more explicit by using the names emitInitNative and emitInitScript. The other emit functions are also renamed. PR-URL: #14152 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
AsyncResource previously called emitInitNative. Since AsyncResource is just an abstraction on top of the emitEventScript functions, it should call emitInitScript instead. PR-URL: #14152 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
6 participants
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
async_hooks
Rename
inittoemitInitNativeand allemit*Stoemit*Script. I know some are against these purely stylish changes so I included a small consistency fix toAsyncResourceandemitInitthat is much more obvious with the renames.