doc, lib, test: do not re-require needlessly#14244
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added process 
vsemozhetbyt added doc 
vsemozhetbyt commented Jul 15, 2017
lib/internal/process.js Outdated
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 utilis called there unconditionally.
test/sequential/test-util-debug.js Outdated
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
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). |
vsemozhetbyt commented Jul 21, 2017
Landed in 4f87522 |
PR-URL: #14244 Reviewed-By: Alexey Orlenko <[email protected]>
addaleax commented Jul 27, 2017
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 |
vsemozhetbyt commented Jul 27, 2017
8.x backport: #14524 |
Backport-PR-URL: #14524 Backport-Reviewed-By: Anna Henningsen <[email protected]> PR-URL: #14244 Reviewed-By: Alexey Orlenko <[email protected]>
MylesBorins commented Aug 16, 2017
v6.x backport please 😄 |
vsemozhetbyt commented Aug 16, 2017
@MylesBorins |
PR-URL: #14244 Reviewed-By: Alexey Orlenko <[email protected]>
7 participants
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.