Skip to content

Conversation

@kaiyoma
Copy link

What / Why

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.

References

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.
@kaiyomakaiyoma requested a review from a team as a code ownerNovember 20, 2019 17:38
@ljharb
Copy link
Contributor

It seems like a bug that the config would ever be missing.

process.on('exit',function(code){
process.emit('timeEnd','npm')
log.disableProgress()
if(npm.config.loaded&&npm.config.get('timing')){
Copy link
Contributor

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?

constconfig=npm.config||{loaded: false}

And then replacing npm.config with config in the lines below.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me.

Copy link
Author

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) 

@kaiyoma
Copy link
Author

It seems like a bug that the config would ever be missing.

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.

@isaacs
Copy link
Contributor

It seems like a bug that the config would ever be missing.

Any error that is thrown during or before the npmconf.load() callback, but after the process.on('uncaughtException', errorHandler) in bin/npm-cli.js, would get us in this state.

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.

@mikemimikmikemimik added pr: needs tests requires tests before merging semver:patch semver patch level for changes labels Nov 26, 2019
@darcyclarkedarcyclarke removed the pr: needs tests requires tests before merging label Feb 18, 2020
@darcyclarkedarcyclarke added this to the OSS - Sprint 4 milestone Feb 18, 2020
@darcyclarkedarcyclarke added the Release 6.x work is associated with a specific npm 6 release label Feb 18, 2020
@darcyclarkedarcyclarke self-assigned this Feb 18, 2020
@darcyclarkedarcyclarke mentioned this pull request Feb 18, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Release 6.xwork is associated with a specific npm 6 releasesemver:patchsemver patch level for changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@kaiyoma@ljharb@isaacs@darcyclarke@mikemimik@kgetz-arista