Skip to content
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

net: remove usage of require('util') #26807

Closed
wants to merge 1 commit into from
Closed

Conversation

dnlup
Copy link
Contributor

@dnlup dnlup commented Mar 20, 2019

Use require('internal/util/inspect').inspect and
require('internal/util/debuglog').debuglog instead of
require('util').debuglog and require('util').inspect.

Refs: #26546

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

@nodejs-github-bot nodejs-github-bot added the net Issues and PRs related to the net subsystem. label Mar 20, 2019
lib/net.js Outdated Show resolved Hide resolved
lib/net.js Outdated Show resolved Hide resolved
Use `require('internal/util/inspect').inspect` and 
`require('internal/util/debuglog').debuglog` instead of 
`require('util').debuglog` and `require('util').inspect`.

Refs: nodejs#26546
@BridgeAR
Copy link
Member

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 22, 2019
@BridgeAR
Copy link
Member

BridgeAR commented Mar 24, 2019

Landed in e112fb4 🎉

@BridgeAR BridgeAR closed this Mar 24, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 24, 2019
Use `require('internal/util/inspect').inspect` and
`require('internal/util/debuglog').debuglog` instead of
`require('util').debuglog` and `require('util').inspect`.

PR-URL: nodejs#26807
Refs: nodejs#26546
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@BridgeAR BridgeAR reopened this Mar 25, 2019
@BridgeAR
Copy link
Member

This broke master (likely due to a different change that landed in the meanwhile that conflicts with this one) and I had to pull it out again.

@dnlup would you be so kind and rebase this and to take another look? Thanks!

@BridgeAR BridgeAR added blocked PRs that are blocked by other issues or PRs. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Mar 25, 2019
@Trott
Copy link
Member

Trott commented Mar 25, 2019

This commit is still in master and the test is still broken. I'll open a fast-track revert unless somebody else is already doing that. And if we get a fix in before we revert, cool. And if not, we can re-land later.

@dnlup
Copy link
Contributor Author

dnlup commented Mar 25, 2019

@BridgeAR Yes, no problem

@dnlup
Copy link
Contributor Author

dnlup commented Mar 25, 2019

@BridgeAR Rebasing I get this error in node/test/parallel/test-net-access-byteswritten.js:15:

assert.strictEqual(tls.TLSSocket.super_.prototype.bytesWritten, undefined);

TypeError: Cannot read property 'prototype' of undefined

which I think it is caused by using

Object.setPrototypeOf(Socket.prototype, stream.Duplex.prototype);
Object.setPrototypeOf(Socket, stream.Duplex);

instead of

util.inherits(Socket, stream.Duplex);

which sets the super_ property:

https://github.com/nodejs/node/blob/master/lib/util.js#L145

@BridgeAR
Copy link
Member

@dnlup you can change the test as I did in another PR that I closed: 43fa556

@dnlup
Copy link
Contributor Author

dnlup commented Mar 26, 2019

@BridgeAR I would change this too if you're ok with it

const internalUtil = require('internal/util');

to

const { deprecate } = require('internal/util');

since deprecate is the only function used in this module.

@BridgeAR
Copy link
Member

@dnlup sure, go ahead.

@dnlup
Copy link
Contributor Author

dnlup commented Mar 26, 2019

@BridgeAR So I rebased master again, modified the test and it looks good. Only thing is that I should force push again. Would that be ok?

@BridgeAR
Copy link
Member

@dnlup thanks. Normally it would absolutely be fine to force push but I would rather have a new PR for this since I messed up removing the original commit (I pushed to my fork instead of to upstream) and it was instead reverted. So this PR is still referenced by the commit and it should actually be closed and ideally another PR would be opened.

@dnlup
Copy link
Contributor Author

dnlup commented Mar 26, 2019

Closing in favour of #26920

@dnlup dnlup closed this Mar 26, 2019
targos pushed a commit to targos/node that referenced this pull request Mar 27, 2019
Use `require('internal/util/inspect').inspect` and
`require('internal/util/debuglog').debuglog` instead of
`require('util').debuglog` and `require('util').inspect`.

PR-URL: nodejs#26807
Refs: nodejs#26546
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
targos pushed a commit that referenced this pull request Mar 27, 2019
Use `require('internal/util/inspect').inspect` and
`require('internal/util/debuglog').debuglog` instead of
`require('util').debuglog` and `require('util').inspect`.

PR-URL: #26807
Refs: #26546
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@dnlup dnlup deleted the util_net branch January 10, 2020 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked PRs that are blocked by other issues or PRs. net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants