Skip to content
This repository has been archived by the owner on Aug 11, 2022. It is now read-only.

Don't crash when node.requiredBy is missing. #13519

Closed
wants to merge 2 commits into from

Conversation

creationix
Copy link
Contributor

@creationix creationix commented Jul 28, 2016

This fix works for us in that we're unblocked and can use the newer npm on our old codebase with some probably malformed packages.

$ ls -lh node_modules
...
lrwxrwxrwx  1 creationix admins   19 Jul 28 20:09 thrift -> thrift-performance/
drwxrwsr-x  5 creationix admins 4.0K Jul 28 20:31 thrift-performance
...

node_modules/thrift-performance/package.json:

{
  "name": "thrift-performance",
  "description": "node-thrift",
  "version": "0.6.0dev5",
  "author": "Wade Simmons <wade@wades.im>",
  "directories" : { "lib" : "./lib/thrift" },
  "main": "./lib/thrift",
  "engines": { "node": ">= 0.2.4" },
  "dependencies": {
    "node-int64": "0.2.x"
  }
}

There may be a better way to handle this edge case, but this patch at least prevents the crash and reports something.

If a package has malformed metadata, this field is sometimes missing.
@creationix
Copy link
Contributor Author

creationix commented Jul 28, 2016

Hmm, I'm finding more places where this missing node.requiredBy property is causing crashes. It might be best to figure out the root cause of it being missing. Let me know how I can help you help me track that down.

@iarna
Copy link
Contributor

iarna commented Aug 11, 2016

Yeah, it's really not supposed to be possible for it to be missing.

@iarna
Copy link
Contributor

iarna commented Aug 11, 2016

I ... don't get a crash with the example you laid out up there. What command are you running?

I do get empty results from npm outdated–I would have sworn we'd fixed this or had a branch for fixing it, but apparently not.

@creationix
Copy link
Contributor Author

I wish I could reduce the test case. I do know that the npm that comes with node 0.10.x doesn't have this problem and the one that comes with node 6.x does. Sorry if that doesn't help much, there have probably been a ton of npm changes in the intervening years.

Is there anything you would like me to test or look for in our code base? The symlink was my initial guess, but it might be something different.

@iarna iarna changed the base branch from master to release-next October 3, 2016 23:27
@iarna iarna modified the milestone: next Oct 3, 2016
@iarna iarna force-pushed the release-next branch 15 times, most recently from 040beea to 5b4e3f3 Compare October 7, 2016 02:55
@zkat zkat removed the in-progress label Oct 17, 2016
@zkat zkat added this to the 4.x milestone Oct 17, 2016
@zkat zkat removed the review label Oct 20, 2016
zkat pushed a commit that referenced this pull request Oct 20, 2016
If a package has malformed metadata, this field is sometimes missing.

PR-URL: #13519
Credit: @creationix
Reviewed-By: @iarna
@zkat
Copy link
Contributor

zkat commented Oct 20, 2016

This has been merged into release-next and will be included in npm@4

@zkat zkat closed this Oct 20, 2016
iarna pushed a commit that referenced this pull request Oct 20, 2016
If a package has malformed metadata, this field is sometimes missing.

PR-URL: #13519
Credit: @creationix
Reviewed-By: @iarna
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants