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

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

Merged
merged 14 commits into from
Oct 16, 2023

Conversation

NeonArray
Copy link
Contributor

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.

@NeonArray NeonArray requested a review from a team as a code owner October 13, 2023 16:33
@wraithgar
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

NeonArray commented Oct 13, 2023

@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.

@@ -113,12 +113,14 @@ const getter = ({ data, key }) => {
} else {
// if can't find any more values, it means it's just over
// and there's nothing to return
if (!_data[k]) {
if (Object.hasOwn(_data, k) && !_data[k]) {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Screenshot 2023-10-16 at 10 46 20 AM

@wraithgar
Copy link
Member

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?

const value = _data[k]
if (typeof value === 'boolean' || (value === '' && Object.hasOwn(_data, k))) {
_data = value
} else if (value === undefined) {
Copy link
Member

@wraithgar wraithgar Oct 16, 2023

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@wraithgar wraithgar Oct 16, 2023

Choose a reason for hiding this comment

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

> 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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That works, change has been made

@wraithgar
Copy link
Member

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
Copy link
Member

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

@NeonArray
Copy link
Contributor Author

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.

Copy link
Member

@wraithgar wraithgar left a comment

Choose a reason for hiding this comment

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

The new pkg test outlines the fix perfectly.

@wraithgar wraithgar merged commit 35c92fe into npm:latest Oct 16, 2023
18 of 20 checks passed
@wraithgar
Copy link
Member

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

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

Successfully merging this pull request may close these issues.

2 participants