chore(refactor): refactor exit handler and tests #3482
Conversation
|
This is done, with a more meaningful comment explaining why it's in there |
wraithgar
force-pushed
the
gar/exit-handler-mark-two
branch
4 times, most recently
from
6ba68c9 to
44e234d
Compare
darcyclarke
added
the
Release 7.x
lib/base-command.js
Outdated
| @@ -75,7 +75,6 @@ class BaseCommand { | |||
| } | |||
|
|
|||
| async setWorkspaces (filters) { | |||
| // TODO npm guards workspaces/global mode so we should use this.npm.prefix? | |||
There was a problem hiding this comment.
is this TODO not something we still need to do? is this an answered question already?
There was a problem hiding this comment.
No, the entire approach to npm.prefix vs npm.localPrefix etc needs addressing and I'd rather leave these comments in until we actually properly audit and review how we're using those.
| this.started = Date.now() | ||
| this.command = null | ||
| this.commands = proxyCmds | ||
| this.timings = timings | ||
| this.timers = timers | ||
| this.perfStart() |
There was a problem hiding this comment.
this is a great intermediate step to avoid the singleton issue 🎉
| process.on('exit', code => { | ||
| // process.emit is synchronous, so the timeEnd handler will run before the | ||
| // unfinished timer check below |
There was a problem hiding this comment.
👏 thank you for calling this out, it's a super nice reminder!
wraithgar
force-pushed
the
gar/exit-handler-mark-two
branch
from
44e234d to
f828c5f
Compare
|
rebasing fixed the conflicts w/o having to change anything. PHEW |
* npm mock logger writes to npm.log.record too now * No more extra process.exit from within the process `exit` event handle. * No more `exit()` function. Logic is rolled up into the exit handler. * Now there is only an exit handler and an exit event listener. `lib/utils/perf.js` was rolled up into npm.js itself. Unfortunately the tests were written in such a way that any further refactoring of the exit handler was going to require also rewriting the tests. Fortunately NOW the tests are interacting with the exit handler in a way that shouldn't require them to be rewritten AGAIN if we change the internals of the exit handler. PR-URL: #3482 Credit: @wraithgar Close: #3482 Reviewed-by: @nlf
wraithgar
force-pushed
the
gar/exit-handler-mark-two
branch
from
f828c5f to
efc4313
Compare
exitevent handle.exit()function. Logic is rolled up into the exit handler.lib/utils/perf.jswas rolled up into npm.js itself.Unfortunately the tests were written in such a way that any further
refactoring of the exit handler was going to require also rewriting the
tests. Fortunately NOW the tests are interacting with the exit handler
in a way that shouldn't require them to be rewritten AGAIN if we change
the internals of the exit handler.
Needs #3479 to land first