Skip to content
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

Gracefully handle "Cannot read property .match of undefined" #1009

Closed

Conversation

danielleadams
Copy link
Contributor

@danielleadams danielleadams commented Mar 11, 2020

What / Why

There's a bug in which an npm command will raise an error Cannot read property .match on undefined and exit the script with an error. There are a few open issues:

I've run into this with npm prune, but I was unable to reproduce it while looking into it. This doesn't fix the root of the issue, which it appears that empty parameters are being generated in the lock file something like this:

"package-a": {
  "dependencies": {
    "package-b": {}
  }
}

And causing an undefined error here: https://github.com/npm/cli/blob/latest/lib/install/inflate-shrinkwrap.js#L87

Because there is no "version" key that is passed in here: https://github.com/npm/cli/blob/latest/lib/install/inflate-shrinkwrap.js#L99 (which is returning an undefined).

Solution

This catches a dependency if it's empty and displays an error message to the user to regenerate the package-lock.json or npm-shrinkwrap.json.

TODO

  • write tests
  • split out the messaging based on the lockfile type if requested

References

  • n/a

@danielleadams danielleadams requested a review from a team as a code owner March 11, 2020 19:04
@danielleadams danielleadams changed the title Error msg for match undefined Guard empty dependency objects and raise error Mar 11, 2020
@danielleadams danielleadams changed the title Guard empty dependency objects and raise error Gracefully handle "Cannot read property .match of undefined" Mar 11, 2020
@ruyadorno ruyadorno added pr: needs tests requires tests before merging Release 6.x work is associated with a specific npm 6 release semver:patch semver patch level for changes labels Mar 24, 2020
@danielleadams danielleadams force-pushed the error-msg-for-match-undefined branch 3 times, most recently from 4c2a0e8 to 3d02e27 Compare April 1, 2020 14:12
@ruyadorno ruyadorno removed the pr: needs tests requires tests before merging label Apr 3, 2020
Copy link
Contributor

@ruyadorno ruyadorno left a comment

Choose a reason for hiding this comment

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

awesome! 🎉 thanks @danielleadams

@ruyadorno
Copy link
Contributor

Sorry for the delay @danielleadams I was meant to write back to you after the last release - I tried landing this in v6.14.5 but found out that there's some broken tests, basically it seems like the commit got the green light because CI just skipped all tests last time 😭

Copy link
Contributor

@ruyadorno ruyadorno left a comment

Choose a reason for hiding this comment

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

Tests are currently failing

@ruyadorno
Copy link
Contributor

btw, if you rebase on top of current latest branch it should fix the problem with CI 😊

@danielleadams
Copy link
Contributor Author

oh whoops! Ok.. let me rebase and see if it fixes, or otherwise fix it.

@danielleadams danielleadams force-pushed the error-msg-for-match-undefined branch 2 times, most recently from b29be19 to 74305db Compare June 11, 2020 13:42
@darcyclarke darcyclarke added this to the OSS - Sprint 8 milestone Jun 11, 2020
Copy link
Contributor

@ruyadorno ruyadorno left a comment

Choose a reason for hiding this comment

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

awesome 🎉 thanks again @danielleadams 😊

Copy link
Contributor

@ruyadorno ruyadorno left a comment

Choose a reason for hiding this comment

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

digging a bit more into the output from each of the CI targets, there's actually a bunch of silent failures in the form of:

 npm ERR! cb() never called!

npm ERR! This is an error with npm itself. Please report this error at:
npm ERR!     <https://npm.community>

npm ERR! A complete log of this run can be found in:
npm ERR!     C:\npm\cache\_logs\2020-06-11T20_02_29_720Z-debug.log

which seems to imply the broken chain of promise is still an issue 😞

@danielleadams danielleadams force-pushed the error-msg-for-match-undefined branch from be3c45b to 5455e0b Compare July 7, 2020 23:21
@danielleadams
Copy link
Contributor Author

@ruyadorno I made changes to how errors are handling... instead of importing and using the error handler, I returned a failed promise with an error from the if statement. The tests pass, which tells me that the error is passed up the call stack and still handled. Let me know what you think 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release 6.x work is associated with a specific npm 6 release semver:patch semver patch level for changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants