Node: lib: reduce internal usage of public require('util')

[joyeecheung]

A few patterns (modulo destructuring):

1. Replace require('util').inherits() usages under lib: https://github.com/nodejs/node/issues/24395

// Change this
util.inherits(A, B);

// To this
Object.setPrototypeOf(A.prototype, B.prototype);
Object.setPrototypeOf(A, B);

1. Replace require('util').inspect with require('internal/util/inspect').inspect
2. Replace require('util').format with require('internal/util/inspect').format
3. Replace require('util').debuglog with require('internal/util/debuglog').debuglog
4. Replace require('util').types with require('internal/util/types')
5. Replace require('util').deprecate with require('internal/util').deprecate
6. Replace require('util').promisify with require('internal/util').promisify

List of JS files that can be refactored:

| Refactored | Subsystem | Files | PR |
|---|---|---|---|
|

• - [x]

| assert | lib/assert.js#L32, lib/internal/assert/assertion_error.js#L3 | #26762, #26750 |
|

• - [x]

| bootstrap | lib/internal/bootstrap/node.js#L192 | #26547 |
|

• - [x]

| child_process | lib/child_process.js#L24, lib/internal/child_process.js#L21 | #26769, #26773 |
|

• - [x]

| console | lib/internal/console/constructor.js#L18 | #26777 |
|

• - [x]

| dgram | lib/dgram.js#L48 | #26770 |
|

• - [x]

| domain | lib/domain.js#L29 | |
|

• - [x]

| encoding | lib/internal/encoding.js#L330 | #26779 |
|

• - [x]

| errors | lib/internal/error-serdes.js#L83, lib/internal/errors.js#L210 | #26782, #26781 |
|

• - [x]

| fs | lib/internal/fs/utils.js#L15 | #26783 |
|

• - [x]

| http | lib/_http_agent.js#L25, lib/_http_common.js#L39, lib/_http_outgoing.js#L26, lib/_http_server.js#L24 | #26548 |
|

• - [x]

| http2 | lib/internal/http2/core.js#L21 | #26789 |
|

• - [x]

| https | lib/https.js#L28 | #26772 |
|

• - [x]

| inspector | lib/inspector.js#L19 | |
|

• - [x]

| lib | lib/internal/policy/manifest.js#L7 | #26833 |
|

• - [ ]

| module | lib/internal/modules/cjs/loader.js#L26, lib/internal/modules/esm/create_dynamic_module.js#L4, lib/internal/modules/esm/loader.js#L21, lib/internal/modules/esm/module_map.js#L7, lib/internal/modules/esm/translators.js#L22 | #26802, #26803, #26804, #26805, #26806 |
|

• - [ ]

| net | lib/net.js#L26 | #26807 |
|

• - [x]

| process | lib/internal/process/per_thread.js#L18 | #26817 |
|

• - [x]

| readline | lib/readline.js#L35 | #26818 |
|

• - [ ]

| repl | lib/internal/repl/history.js#L7, lib/repl.js#L55 | #26819 #26820 |
|

• - [x]

| stream | lib/internal/js_stream_socket.js#L4, lib/internal/streams/buffer_list.js#L4, lib/internal/streams/lazy_transform.js#L7, lib/stream.js#L46 | #26698 |
|

• - [x]

| timers | lib/internal/timers.js#L19, lib/timers.js#L43 | |
|

• - [x]

| tls | lib/_tls_wrap.js#L30 | #26747 |
|

• - [x]

| trace_events | lib/trace_events.js#L22 | #26822 |
|

• - [x]

| tty | lib/tty.js#L24 | #26797 |
|

• - [x]

| url | lib/internal/url.js#L3 | #26808 |
|

• - [ ]

| vm | lib/internal/vm/source_text_module.js#L3, lib/vm.js#L35 | #26716 |
|

• - [x]

| worker | lib/internal/main/worker_thread.js#L46, lib/internal/worker/io.js#L17 | #26814, #26810 |
|

• - [ ]

| zlib | lib/zlib.js#L38 | #26716 |

[micheleriva]

I can work on it :tada:

[joyeecheung]

@micheleriva That's great! If possible please try to refactor each file in single commit and submit them as separate PRs, otherwise the PR is bound to require manual backport.

[SimonSchick]

@joyeecheung Just curious, in many cases util.inherits could be replaced by class ... extends ... instead, is there a reason setPrototypeOf should be used instead?

[micheleriva]

@joyeecheung ok no prob! So I'll close my PR #26552 in flavour of single PRs.

[joyeecheung]

@SimonSchick For public ES5-style classes we have to support instantiation without new so we cannot refactor them to ES6 classes.

[SimonSchick]

I was talking about those that do not support instantiation without new.

Might also be worth mentioning that we dealt with the same issue in sequelize using Proxys https://github.com/sequelize/sequelize/blob/master/lib/utils/classToInvokable.js#L9
I'm not sure what the exact performance penalties are tho.

[dnishar1]

I can work on it!

[joyeecheung]

I was talking about those that do not support instantiation without new.

@SimonSchick I guess if you are sure those do not support instantiation without new, those could be refactored if the tests are happy (probably also needs a CITGM run anyway because undoing util.inherits also removes the hacky super_ property on the classes)

[gengjiawen]

@joyeecheung Maybe make this task a table like this https://github.com/facebook/jest/issues/7807#issue-406936815

[sasurau4]

I'll work on lib/assert.js :+1:

[dnlup]

@joyeecheung In lib/domain.js the only usage of require('util') is to call
https://github.com/nodejs/node/blob/d57d09905e30227936ac80ff547da27181566082/lib/domain.js#L198
of which I don't see any implementation in lib/internal/util, except for a couple of methods in lib/internal/util/comparison. Should the file be left as is or one of those methods should be used instead?

[joyeecheung]

@dnlup Yeah we can leave lib/domain.js as is. I'll remove it from OP.

[dnlup]

@joyeecheung Great, thank you

[toshi1127]

@joyeecheung
I worked on it !!
lib/child_process.js: #26769
lib/internal/assert/assertion_error.js: #26750
lib/internal/http2/core.js: #26784

[Naturalclar]

I'd like to contribute!
I'll work on lib/internal/http2/core.js

[nd-02110114]

I'll work on lib/tty.js#L24

[toshi1127]

I worked on lib/internal/fs/utils.js: #26783

[ZYSzys]

Hi, everyone, thanks for your contributions first.

It would be better if you can use the exact subsystem instead of simple lib in PR title(and commit message), then we can track easier :)

[dnlup]

@ZYSzys I know, sorry but I could not find any reference to the right subsystem here and saw my first pull request build failing because of the commit message.
What pattern should I use for the internal subsystem? I apologize again.

[ZYSzys]

@dnlup Most can be referenced from table above now since I changed the refactored list to table which contains susbsystem. If no related subsystem found in internal, let's just use lib :)

[dnlup]

@joyeecheung I think the same situation of lib/domain.js applies to:

• lib/inspector
  https://github.com/nodejs/node/blob/d57d09905e30227936ac80ff547da27181566082/lib/inspector.js#L71

[joyeecheung]

@dnlup thanks, removed

[toshi1127]

@joyeecheung
sorry, lib/internal/timers.js and lib/timers.js were already refactored.

[Naturalclar]

I've worked on lib/internal/policy/manifest.js #26833

[DavidLi119]

@joyeecheung Hi, are there any more files that need refactored? If there are, may I work on some?

[gsilvapt]

I believe all the files mentioned in the first message are already fixed and/or have open PRs that handle the issue at hands. Can we tick those files of the table to avoid confusion?

[karansapolia]

Was looking for issues to work on and noticed that required changes have already been made and merged for timers in lib/internal/timers.js and lib/timers.js. Corresponding PRs can be updated in the original post.

[lesagi]

Any reason that this is still open?
Seems like everything was refactored? (given what @karansapolia said).

[BridgeAR]

I can only find a single require('util') in lib at the moment that could be replaced. I'll open a PR for that and close this issue as resolved.