fix: Add check to pkg command to deal with empty values

[NeonArray]

If your package.json contains an empty value such as:

{
    "author": "",
    "name": "foo"
}

Executing, npm pkg get name author will return undefined. You'd expect it to return "foo".

This is because the args is being used as an index when returning a single value. This patch fixes this and adds two new tests to the pkg command.

[wraithgar]

I think there's a bad assumption being made here (i.e. the original code) where if only one item was returned then only one item was asked for.

The "length" check is more intended for the use case of "I asked for one attribute but the value of it is an array", e.g.

$ npm pkg get dependencies
{
  "@npmcli/promise-spawn": "^6.0.0",
  "lru-cache": "^10.0.1",
  "npm-pick-manifest": "^9.0.0",
  "proc-log": "^3.0.0",
  "promise-inflight": "^1.0.1",
  "promise-retry": "^2.0.1",
  "semver": "^7.3.5",
  "which": "^3.0.0"
}

I would think that if I asked for multiple attributes, I will always get back the long form, even if some of them are empty.

npm pkg get is supposed to be the "local" version of npm view, and if you compare the two

~/D/n/c/b/main (latest|✔) $ npm pkg get name foo
undefined
~/D/n/c/b/main (latest|✔) $ npm view npm name foo
npm
~/D/n/c/b/main (latest|✔) $ npm pkg get name version
{
  "name": "npm",
  "version": "10.2.0"
}
~/D/n/c/b/main (latest|✔) $ npm view npm name version
name = 'npm'
version = '10.2.0'

You'll see both exhibiting this bug. Both of them use queryable.js and they are the only things that use it, so I think at least one bug is in there. At a glance I'd suspect line 116 which is doing

if (!_data[k]) {
  return undefined
}

Perhaps that should be a Object.hasOwnProperty check?

Then we can also make sure that if the user asked for more than one attribute, we always return the long form.

[NeonArray]

Very good points @wraithgar. I was conflicted whether or not it should return the empty string or not, but I should have searched deeper for the core issue. I'll take a look and adjust the code/PR.

[NeonArray]

@wraithgar I've made some modifications and added a test for the view command. I had to generate snapshots for Tap, do I need to commit the files it generates? I've never used the snapshot stuff before so wasn't sure.

Edit:
I just realized my view test probably isn't doing what I need.

[wraithgar]

The falsey check here was a bad idea in the first place. What if the value of node-gyp is false? Shouldn't I get that back?

[NeonArray]

I initially didn't have that check there, but without it a bunch of other tests fail.

[wraithgar]

Yes when you run npm run snap the changes it makes should be included in your commit.

As I commented inline, the entire premise of doing a falsey check here is wrong, I think. Do you agree?

[wraithgar]

if value === undefined and value = _data[k] then returning _data[k] would return undefined anyways. Not sure what this check gains us.

[NeonArray]

You're right, I did overcomplicate that. The undefined check is needed since other functionality/tests expect it to be returned and not just set to _data.

I simplified it so now it only returns undefined, else just sets up _data = value

[wraithgar]

> The undefined check is needed since other functionality/tests expect it to be returned and not just set to _data.

Can you show me where this is happening? That still seems very counterintuitive.

[wraithgar]

oooooh I see it now. we're not returning otherwise. My bad!

[wraithgar]

OK I think we're getting somewhere. I think this check should be JUST a hasOwn, and if that's false we return undefined. Otherwise we let it fall through.

[NeonArray]

That works, change has been made

[wraithgar]

It looks like the early return has to do w/ the accumulator that keeps a running tab of what label is, and that's the short circuit to prevent 'not defined' errors.

Unpacking that is a larger issue than this PR imo.

[wraithgar]

The two failing tests are flaky, not related to this PR

[NeonArray]

The two failing tests are flaky, not related to this PR

Ah ok, I was trying to replicate locally but the tests would all pass so was a little confused.

[wraithgar]

The new pkg test outlines the fix perfectly.

[wraithgar]

This will go out w/ the next npm release.

If you need this in npm 9 you can cherry pick 35c92fe and make a new PR to the release/v9 branch