-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
fix: check npm.config
before accessing its members
#508
Conversation
Sometimes, `npm.config` can be missing entirely, but there are several places where `npm.config.foo` is accessed blindly, resulting in these kinds of errors and stack traces: TypeError: Cannot read property 'get' of undefined at errorMessage (.../lib/utils/error-message.js:38:39) ... TypeError: Cannot read property 'loaded' of undefined at exit (.../lib/utils/error-handler.js:97:27) ... LBYL by checking `npm.config` first. Addresses a small part of #502.
It seems like a bug that the config would ever be missing. |
@@ -36,7 +36,7 @@ process.on('timing', function (name, value) { | |||
process.on('exit', function (code) { | |||
process.emit('timeEnd', 'npm') | |||
log.disableProgress() | |||
if (npm.config.loaded && npm.config.get('timing')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could dry this up by adding something like this near the top, no?
const config = npm.config || { loaded: false }
And then replacing npm.config
with config
in the lines below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this approach seems to introduce more stack traces and more errors. Is this what we want?
$ ./bin/npx-cli.js foo
Command failed: node.exe npm-cli.js config get cache --parseable
Error: Exit prior to config file resolving.
at errorHandler (...\lib\utils\error-handler.js:149:16)
at ...\bin\npm-cli.js:150:20
at get (...\lib\config.js:171:3)
at EventEmitter.config (...\lib\config.js:68:14)
at Object.commandCache.(anonymous function) (...\lib\npm.js:156:13)
at EventEmitter.<anonymous> (...\bin\npm-cli.js:131:30)
at process._tickCallback (internal/process/next_tick.js:61:11)
npm ERR! Exit prior to config file resolving.
Error: npm.load() required
at Object.get (...\lib\npm.js:59:13)
at errorHandler (...\lib\utils\error-handler.js:207:14)
at ...\bin\npm-cli.js:150:20
at get (...\lib\config.js:171:3)
at EventEmitter.config (...\lib\config.js:68:14)
at Object.commandCache.(anonymous function) (...\lib\npm.js:156:13)
at EventEmitter.<anonymous> (...\bin\npm-cli.js:131:30)
at process._tickCallback (internal/process/next_tick.js:61:11)
npm ERR! npm.load() required
...\lib\npm.js:59
throw new Error('npm.load() required')
^
Error: npm.load() required
at Object.get (...\lib\npm.js:59:13)
at process.errorHandler (...\lib\utils\error-handler.js:207:14)
at process.emit (events.js:198:13)
at process._fatalException (internal/bootstrap/node.js:496:27)
Agreed, but I can't seem to figure out the root cause right now. There's something funky with directories with spaces (on Windows 10) not being quoted correctly, which causes a cascade of errors. This PR is simply for fixing the issues with accessing a missing config. |
Any error that is thrown during or before the It's a valid situation to defend against, and failing better is better than failing worse, but yes, I agree it's definitely an indication of another problem. |
What / Why
Sometimes,
npm.config
can be missing entirely, but there are severalplaces where
npm.config.foo
is accessed blindly, resulting in thesekinds of errors and stack traces:
LBYL by checking
npm.config
first. Addresses a small part of #502.References