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

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

Closed
joyeecheung opened this issue Mar 9, 2019 · 29 comments
Closed

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

joyeecheung opened this issue Mar 9, 2019 · 29 comments
Labels
good first issue Issues that are suitable for first-time contributors. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. util Issues and PRs related to the built-in util module.

Comments

@joyeecheung
Copy link
Member

joyeecheung commented Mar 9, 2019

A few patterns (modulo destructuring):

  1. Replace require('util').inherits() usages under lib: Remove util.inherits usage internally? #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
assert lib/assert.js#L32, lib/internal/assert/assertion_error.js#L3 #26762, #26750
bootstrap lib/internal/bootstrap/node.js#L192 #26547
child_process lib/child_process.js#L24, lib/internal/child_process.js#L21 #26769, #26773
console lib/internal/console/constructor.js#L18 #26777
dgram lib/dgram.js#L48 #26770
domain lib/domain.js#L29
encoding lib/internal/encoding.js#L330 #26779
errors lib/internal/error-serdes.js#L83, lib/internal/errors.js#L210 #26782, #26781
fs lib/internal/fs/utils.js#L15 #26783
http lib/_http_agent.js#L25, lib/_http_common.js#L39, lib/_http_outgoing.js#L26, lib/_http_server.js#L24 #26548
http2 lib/internal/http2/core.js#L21 #26789
https lib/https.js#L28 #26772
inspector lib/inspector.js#L19
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
process lib/internal/process/per_thread.js#L18 #26817
readline lib/readline.js#L35 #26818
repl lib/internal/repl/history.js#L7, lib/repl.js#L55 #26819 #26820
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
timers lib/internal/timers.js#L19, lib/timers.js#L43
tls lib/_tls_wrap.js#L30 #26747
trace_events lib/trace_events.js#L22 #26822
tty lib/tty.js#L24 #26797
url lib/internal/url.js#L3 #26808
vm lib/internal/vm/source_text_module.js#L3, lib/vm.js#L35 #26716
worker lib/internal/main/worker_thread.js#L46, lib/internal/worker/io.js#L17 #26814, #26810
zlib lib/zlib.js#L38 #26716
@joyeecheung joyeecheung added help wanted Issues that need assistance from volunteers or PRs that need help to proceed. util Issues and PRs related to the built-in util module. good first issue Issues that are suitable for first-time contributors. labels Mar 9, 2019
@micheleriva
Copy link

I can work on it 🎉

@joyeecheung
Copy link
Member Author

@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.

anantsinha added a commit to anantsinha/node that referenced this issue Mar 10, 2019
Remove public require('util') and replace util.isFunction with internal/util/isFunction

Fixes: nodejs#26546
@SimonSchick
Copy link
Contributor

@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
Copy link

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

@joyeecheung
Copy link
Member Author

joyeecheung commented Mar 10, 2019

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

@SimonSchick
Copy link
Contributor

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.

pull bot pushed a commit to Rachelmorrell/node that referenced this issue Mar 12, 2019
PR-URL: nodejs#26548
Refs: nodejs#26546
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
joyeecheung added a commit that referenced this issue Mar 12, 2019
PR-URL: #26547
Refs: #26546
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@dnishar1
Copy link

I can work on it!

@joyeecheung
Copy link
Member Author

joyeecheung commented Mar 13, 2019

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)

BridgeAR pushed a commit that referenced this issue Mar 13, 2019
PR-URL: #26547
Refs: #26546
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
refack pushed a commit to refack/node that referenced this issue Mar 14, 2019
PR-URL: nodejs#26548
Refs: nodejs#26546
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
BridgeAR pushed a commit that referenced this issue Mar 14, 2019
PR-URL: #26547
Refs: #26546
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@gengjiawen
Copy link
Member

@joyeecheung Maybe make this task a table like this jestjs/jest#7807 (comment)

joyeecheung added a commit to joyeecheung/node that referenced this issue Mar 18, 2019
dnlup added a commit to dnlup/node that referenced this issue Mar 18, 2019
Remove the usage of public require('util'), as described in issue:
  nodejs#26546
targos pushed a commit that referenced this issue 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]>
targos pushed a commit that referenced this issue Mar 27, 2019
Use `require('internal/util/inspect').format` instead of
`require('util').format`.

PR-URL: #26817
Refs: #26546
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
targos pushed a commit that referenced this issue Mar 27, 2019
Replace `require('util').inspect` and `require('util').format` with
`require('util/internal/inspect').inspect` and
`require('util/internal/inspect').format` in `lib/internal/errors.js`.

PR-URL: #26782
Refs: #26546
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Mar 27, 2019
Remove internal usage of `require('util').inspect`.

PR-URL: #26781
Refs: #26546
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
targos pushed a commit that referenced this issue Mar 27, 2019
PR-URL: #26583
Refs: #26546
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this issue Mar 27, 2019
Refs: #26546

PR-URL: #26583
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this issue Mar 27, 2019
This patch:

- Moves the timer callback initialization into bootstrap/node.js,
  documents when they will be called, and make the dependency on
  process._tickCallback explicit.
- Moves the initialization of tick callbacks and timer callbacks
  to the end of the bootstrap to make sure the operations
  done before those initializations are synchronous.
- Moves more internals into internal/timers.js from timers.js.

PR-URL: #26583
Refs: #26546
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this issue Mar 28, 2019
Backport-PR-URL: #26650
PR-URL: #26548
Refs: #26546
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
targos pushed a commit that referenced this issue Mar 28, 2019
PR-URL: #26810
Refs: #26546
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
targos pushed a commit that referenced this issue Mar 28, 2019
PR-URL: #26814
Refs: #26546
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
ZYSzys pushed a commit that referenced this issue Mar 29, 2019
Use `require('internal/util/inspect').inspect` and
`require('internal/util/debuglog').debuglog` instead of
`require('util').inspect` and `require('util').debuglog`.

Refs: #26546

PR-URL: #26820
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
targos pushed a commit that referenced this issue Mar 30, 2019
Use `require('internal/util/inspect').inspect`,
`require('internal/util/debuglog').debuglog`,
`require('internal/util').deprecate` and `Object.setPrototypeOf` instead
of `require('util')`.
Fix test in `test/parallel/test-net-access-byteswritten.js` to do not
check the `super_` property that was set when using
`require('util').inherits`.

Refs: #26546
Refs: #26896

PR-URL: #26920
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit that referenced this issue Mar 30, 2019
Backport-PR-URL: #26650
PR-URL: #26548
Refs: #26546
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
targos pushed a commit that referenced this issue Mar 30, 2019
PR-URL: #26810
Refs: #26546
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
targos pushed a commit that referenced this issue Mar 30, 2019
PR-URL: #26814
Refs: #26546
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
BethGriggs pushed a commit that referenced this issue Apr 5, 2019
Use `require('internal/util/inspect').inspect` and
`require('internal/util/debuglog').debuglog` instead of
`require('util').inspect` and `require('util').debuglog`.

Refs: #26546

PR-URL: #26820
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
BethGriggs pushed a commit that referenced this issue Apr 5, 2019
Use `require('internal/util/inspect').inspect`,
`require('internal/util/debuglog').debuglog`,
`require('internal/util').deprecate` and `Object.setPrototypeOf` instead
of `require('util')`.
Fix test in `test/parallel/test-net-access-byteswritten.js` to do not
check the `super_` property that was set when using
`require('util').inherits`.

Refs: #26546
Refs: #26896

PR-URL: #26920
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
dnlup added a commit to dnlup/node that referenced this issue Apr 10, 2019
Use `require('internal/util/debuglog').debuglog` instead of 
`require('util').debuglog` in 
`lib/internal/modules/esm/create_dynamic_module.js`.

Refs: nodejs#26546
BridgeAR pushed a commit to BridgeAR/node that referenced this issue Apr 10, 2019
Use `require('internal/util/debuglog').debuglog` instead of
`require('util').debuglog` in
`lib/internal/modules/esm/create_dynamic_module.js`.

PR-URL: nodejs#26803
Refs: nodejs#26546
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
BethGriggs pushed a commit that referenced this issue Apr 17, 2019
Use `require('internal/util/inspect').format` instead of
`require('util').format`.

Refs: #26546

PR-URL: #26822
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
ZYSzys pushed a commit that referenced this issue Apr 21, 2019
PR-URL: #27285
Refs: #26546
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
MylesBorins pushed a commit that referenced this issue May 16, 2019
Use `require('internal/util/inspect').format` instead of
`require('util').format`.

Refs: #26546

PR-URL: #26822
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
@karansapolia
Copy link

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
Copy link

lesagi commented Sep 28, 2019

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

@BridgeAR
Copy link
Member

BridgeAR commented Oct 3, 2019

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.

@BridgeAR BridgeAR closed this as completed Oct 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues that are suitable for first-time contributors. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

No branches or pull requests