fix: account for packument with no versions #81
Conversation
Resolve line-length lint error blocking npm#81
|
Linting and tests need to pass. |
wraithgar
changed the title
|
@wraithgar This lint issue is easy, but based on the solution in this PR and your implementation, I don't see how to test Advisory.js#109 sufficiently. This fix appears to be needed because the code assumes objects will exist that don't and so that bit of code isn't setup to be testable. I'd love to help more as this is blocking our pipelines on |
| @@ -242,7 +242,7 @@ class Advisory { | |||
| // check the dependency of this version on the vulnerable dep | |||
| // if we got a version that's not in the packument, fall back on | |||
| // the spec provided, if possible. | |||
| const mani = this[_packument].versions[version] || { | |||
| const mani = this[_packument].versions && this[_packument].versions[version] ? this[_packument].versions[version] : { | |||
There was a problem hiding this comment.
hi @yadirhb, thanks a lot for contributing to this issue, if possible could you try to fix this linting issue so git flow will pass?
There was a problem hiding this comment.
It just needs line-breaks:
const mani = this[_packument].versions && this[_packument].versions[version] ?
this[_packument].versions[version] :
{
I submitted this PR a few days ago to address that. Should I setup a new one here to get this moving?
There was a problem hiding this comment.
There was a problem hiding this comment.
tbh I'm not sure if original author is available to accept your changes or if opening a new PR would help :/
…l to object These changes are from npm#81, but address the lint error. I've added a test, but was still wasn't able to satisfy the 100% requirement. If someone else can't improve on this to that end, should we consider reducing the requirement to 99% so everyone depending on this can get past their `npm i` issues, blocking pipelines all over.
|
I've put up a new PR to consider as an alternative to this one: #83 |
|
Packuments only have no versions when they've been unpublished, and wouldn't be in your tree to be send to this module in the first place. |
|
"wouldn't be in your tree " @wraithgar I hear your theory, but do you think it's more likely that more than one of us landed here at the same time, for the same reason, to that very line of code; if it weren't running and causing the error we've reported? It may be that it "shouldn't" happen, but if it does, do you want to continue to be responsible for maintaining fragile code, or do you want to AT LEAST let the logical failure move on to be exposed higher up the call-chain? Right now, this code IS the problem and to me, you're coming across very short-termed and extremely unapreciative of the time we're putting in to try and help. I'm open to the possibility that I'm still confused, but it seems a lot less likely to me. Perhaps you're assuming (as the code does), that a scenario will not occur that IS occurring and because the code isn't as robust as it could be, we're here today? |
|
hey @wraithgar true but this happens also when you try to install a package via github link, and previously some package with same name existed in npm (e.g. a fake package), and this is causing issues. Due to this I would suggest to add a support for this optional versions field. Sample case is this package: https://github.com/bitfinexcom/bfx-svc-test-helper, someone put a fake package with same name previously on npm and it got unpublished as expected, but now due to this npm issue we can't install this dependency from github |
|
So is this bug going to be left in the codebase or...? We have multieple pipelines failing on |
|
Just to add another data point, running |
|
The underlying bug is npm/cli#5110, it is not a bug with this module. |
Details: #80
References