doc, lib, test: do not re-require needlessly #14244
Conversation
nodejs-github-bot
added
process
vsemozhetbyt
added
doc
| @@ -1,6 +1,8 @@ | |||
| 'use strict'; | |||
|
|
|||
| const errors = require('internal/errors'); | |||
| const util = require('util'); | |||
There was a problem hiding this comment.
It seems that the intention in this file was to do this require only when really needed so I'm not sure if it's a good idea to require it unconditionally.
There was a problem hiding this comment.
I've checked this. If I don't miss something, this module is required only in the bootstrap_node.js and the function which requires util is called there unconditionally.
test/sequential/test-util-debug.js
Outdated
| @@ -22,13 +22,16 @@ | |||
| 'use strict'; | |||
| const common = require('../common'); | |||
| const assert = require('assert'); | |||
| let spawn; | |||
There was a problem hiding this comment.
I honestly find the original easier in this case, no changes required imho.
test/common/index.js
Outdated
| @@ -26,10 +26,10 @@ const fs = require('fs'); | |||
| const assert = require('assert'); | |||
| const os = require('os'); | |||
| const child_process = require('child_process'); | |||
| const execSync = child_process.execSync; | |||
There was a problem hiding this comment.
could this be made like..
const { execSync, spawn } = require('child_process');?
There was a problem hiding this comment.
Done for all four methods.
|
Rebased. CI: https://ci.nodejs.org/job/node-test-pull-request/9265/ (green). |
|
Landed in 4f87522 |
PR-URL: #14244 Reviewed-By: Alexey Orlenko <[email protected]>
|
This doesn’t land cleanly on 8.x; if you can, please follow the guide and raise a backport PR, if you don’t think it’s worth it let me know/add the |
|
8.x backport: #14524 |
Backport-PR-URL: #14524 Backport-Reviewed-By: Anna Henningsen <[email protected]> PR-URL: #14244 Reviewed-By: Alexey Orlenko <[email protected]>
|
v6.x backport please 😄 |
|
@MylesBorins |
PR-URL: #14244 Reviewed-By: Alexey Orlenko <[email protected]>
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
doc, lib, test
While cached, re-requiring still has some overhead. So I've checked various patterns prone to re-requiring and tried to reduce it.
Lazy loadings are not touched: if there is a chance that a module may not be required at all, potential re-requiring is left as is.
I have a doubt concerning
pummel\test-vm-memleak.js: if it tests leaking inrequireamong other leaks, let me know to revert; otherwise the pummelling can be reduced a bit this way.