Refactor CLI to use npm and yarn instead of third party packages
#3275
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Usage: npm <command>
where <command> is one of:
access, adduser, bin, bugs, c, cache, completion, config,
ddp, dedupe, deprecate, dist-tag, docs, doctor, edit,
explore, get, help, help-search, i, init, install,
install-test, it, link, list, ln, login, logout, ls,
outdated, owner, pack, ping, prefix, profile, prune,
publish, rb, rebuild, repo, restart, root, run, run-script,
s, se, search, set, shrinkwrap, star, stars, start, stop, t,
team, test, token, tst, un, uninstall, unpublish, unstar,
up, update, v, version, view, whoami
npm <command> -h quick help on <command>
npm -l display full usage info
npm help <term> search for help on <term>
npm help npm involved overview
Specify configs in the ini-formatted file:
/Users/felipedeboni/.npmrc
or on the command line via: npm <command> --key value
Config info can be viewed via: npm help config
npm@5.6.0 /Users/felipedeboni/.nvm/versions/node/v9.5.0/lib/node_modules/npm and yarn install v1.5.1
[1/5] Validating package.json...
[2/5] Resolving packages...
success Already up-to-date.
Done in 1.69s. instead of third party packages #3274 #3060
Hypnosphi
added
dependencies
cli
bug
patch:yes
danielduan
approved these changes
Mar 24, 2018
Codecov Report
@@ Coverage Diff @@
## master #3275 +/- ##
==========================================
- Coverage 36.21% 36.18% -0.03%
==========================================
Files 443 444 +1
Lines 9737 9744 +7
Branches 918 901 -17
==========================================
Hits 3526 3526
- Misses 5635 5642 +7
Partials 576 576
Continue to review full report at Codecov.
|
|
@Hypnosphi this is the only obvious thing that I think could cause the linked issue. You worked with the CLI quite a bit. Does this look good? |
|
The passing CLI tests are an extremely good sign |
Hypnosphi
reviewed
Mar 26, 2018
| const packageManager = hasYarn() ? 'yarn' : 'npm'; | ||
|
|
||
| export default async function latestVersion(packageName) { | ||
| const result = spawnSync(packageManager, ['info', packageName, '--json'], { |
There was a problem hiding this comment.
is there any reason not to use ${packageManager} info ${packageName} version?
|
@Hypnosphi it's just to be safe. Because I read |
|
@felipedeboni output without |
|
@Hypnosphi, you're right. Going to open another pull request, to actually fix NPM and also change that behavior. Do you want to check the code first? Commit |
|
#3297 should already have fixed this |
Hypnosphi
added
the
patch:done
Hypnosphi
added a commit
that referenced
this pull request
Mar 29, 2018
Refactor CLI to use `npm` and `yarn` instead of third party packages
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue: #3274 and #3060
What I did
yarn remove latest-versionon thelib/clifolderlib/cli/lib/latest_version.jsand used thehas_yarn.jsto check if yarn is actually available.yarn teston root folder.How to test
Is this testable with jest or storyshots?
I didn't touched on tests, seems that tests are already written.
Does this need a new example in the kitchen sink apps?
No.
Does this need an update to the documentation?
No.
@danielduan you're the man to contact, so can you review if everything is fine here? If not, please let me know and I am going to fix it.