test: use ES6 classes instead of util.inherits#16938
test: use ES6 classes instead of util.inherits#16938tniessen wants to merge 3 commits intonodejs:masterfrom
Conversation
|
AFAICT this can be backported to |
maclover7
left a comment
maclover7
left a comment
There was a problem hiding this comment.
One small comment, nice cleanup!!
|
|
||
| _read(n) { | ||
| setTimeout(() => { | ||
|
|
There was a problem hiding this comment.
While here, may as well remove this blank line?
|
I would like to see a backport for 8.x and 6.x opened before this lands edit: not going to block on it, but would be nice |
|
CI: https://ci.nodejs.org/job/node-test-pull-request/11351/ @MylesBorins I opened a backport PR for 8.x (#16946). It would be nice to land #16947 before backporting this to 6.x. |
| return done(null); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
(while we are here) how about eclosing the class definition inside the common.hasFipsCrypto block as it is used only if the conditional is met?
| util.inherits(Agent, http.Agent); | ||
|
|
||
| Agent.prototype.createConnection = function() { | ||
| const self = this; |
There was a problem hiding this comment.
how about eliminating the need for self by converting the only consuming function to an arrow function?
There was a problem hiding this comment.
I did that in a couple of other places, seems like I missed it here.
There was a problem hiding this comment.
Ahh I remember why I did not do it, because it is simpler to remove the listener this way. Can still refactor it, but it's not pretty.
| class TestStream { constructor() { } } | ||
| util.inherits(TestStream, events.EventEmitter); | ||
| class TestStream extends events.EventEmitter {} | ||
|
|
There was a problem hiding this comment.
same comment as above - confining the scope into the block where it is used.
|
CI failure is unrelated |
|
Given that this is a test cleanup, it has passed CI, and has plenty of signoff, I'm going to go ahead and land. |
PR-URL: #16938 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
|
Landed in 3fe165a |
PR-URL: #16938 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
PR-URL: nodejs#16938 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
PR-URL: nodejs#16938 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
9 participants
As part of the ongoing refactoring to ES6, I replaced many occurrences of
util.inheritswithin our unit tests with ES6 classes, plus some other minor changes to make the code more readable.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test