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

Fixes #13973 #14030 #14032

Closed
wants to merge 1 commit into from
Closed

Conversation

roblg
Copy link
Contributor

@roblg roblg commented Sep 21, 2016

This makes npm show for an unpublished version print empty string, regardless of whether --json is passed. As a (intentional) side effect of making it return empty string for --json, it fixes #14030.

Empty string for --json might be the wrong thing, but it's what npm2 did, and it seemed weird at first blush for --json to print undefined, since I don't think that's valid JSON.

Approach: wrap some stuff towards the end of printData in a --json check, since it doesn't seem like it should apply if we're not printing JSON. The check for if (!msg) was what was ultimately causing the undefined to be printed, and it seems like we should be going into that branch (line 283 in the patch) if we have no msgJson data to begin with.

LMK if there's anything I can clean up here (or if I'm way off base). Thanks!

Commits
13973: npm show w/ undefined version prints 'undefined'
14030: npm show --json w/ semver range prints single result

13973: npm show w/ undefined version prints 'undefined'
14030: npm show --json w/ semver range prints single result
@roblg roblg force-pushed the iss-13973-npm-view-undefined branch from 6ca3389 to f87bcb0 Compare September 21, 2016 22:00
@roblg
Copy link
Contributor Author

roblg commented Sep 22, 2016

Are the appveyor checks expected to pass? It looks like they've been failing a lot (https://ci.appveyor.com/project/npm/npm/history)

@iarna
Copy link
Contributor

iarna commented Oct 3, 2016

Our AppVeyor config currently has some bugs around git paths that stop it from completing successfully (even though we can complete testing on Windows locally).

Copy link
Contributor

@iarna iarna left a comment

Choose a reason for hiding this comment

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

This looks great, thank you!

@iarna iarna added this to the next milestone Oct 3, 2016
@iarna
Copy link
Contributor

iarna commented Oct 6, 2016

This has been merged into the release-next branch and will be in the next release later today.

@iarna iarna closed this Oct 6, 2016
iarna pushed a commit that referenced this pull request Oct 6, 2016
Previously it printed 'undefined' which was particularly problematic when outputting JSON.

Fixes: #13973
Credit: @roblg
Reviewed-By: @iarna
PR-URL: #14032
iarna pushed a commit that referenced this pull request Oct 6, 2016
In npm@2 multiple results were printed with filenames inbetween, resulting
in a stream of plaintext mixed with JSON. This was pretty undesirable.

Previously in, npm@3 the "best" version from the list was picked and shown,
with no facility for getting multiple results.

This changes npm@3 to return an array of matching results.

Fixes: #14030
Credit: @roblg
Reviewed-By: @iarna
PR-URL: #14032
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.

2 participants