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

Treat lack of requirement name as failure to find it #9715

Closed
wants to merge 1 commit into from

Conversation

tmct
Copy link
Contributor

@tmct tmct commented Sep 22, 2015

This should fix #9669

@iarna
Copy link
Contributor

iarna commented Sep 25, 2015

@tmct: npm install http://path/to/tarball.tgz is a valid argument that doesn't have a name.

@iarna
Copy link
Contributor

iarna commented Sep 25, 2015

That being said, markDeps is called in such a way that it absolutely should not be able to be passed a spec that doesn't have a name part. As such, I think this is papering over some larger issue...

@iarna
Copy link
Contributor

iarna commented Sep 25, 2015

And yeah, wow, #9669 is definitely not valid and should be exploding earlier and in a more informative manner. =D

@iarna
Copy link
Contributor

iarna commented Sep 25, 2015

I like this patch by the way – recalculateMetadata (and thus markDeps) need to be resilient to bad data, but to fix #9669 we're going to need an earlier error check I think (before recalculateMetadata is even called).

@tmct
Copy link
Contributor Author

tmct commented Sep 25, 2015

Yes, #9669 is very invalid; even the reporter says so :) I initially dived into this expecting to be adding a more informative error message as othiym23 suggested, but putting in this later defensive check seemed like a good first step (and made the resulting error much more sane!)

I'll have a look tomorrow to see where I'd add the earlier error check.

@iarna iarna added this to the 3.x-next milestone Sep 25, 2015
@iarna iarna modified the milestones: 3.x-next-next, 3.x-next Oct 7, 2015
iarna pushed a commit that referenced this pull request Oct 8, 2015
@iarna
Copy link
Contributor

iarna commented Oct 9, 2015

Hi, thank you for this! It shipped with 3.3.7 just yesterday. =)

@iarna iarna closed this Oct 9, 2015
@tmct
Copy link
Contributor Author

tmct commented Oct 10, 2015

No problem! Great news - thanks for using it :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants