Feature/1487 initialize async api #1488
Conversation
In tests, examples and documentation. The idea is to stop confusion between 'yargs' (the module itself, the singleton, the sync API) and 'y' (a yargs instance). This confusion was sometimes misleading users, and become less and less readable in a context with 2 cohabitating APIs.
yargsa API is at the moment a copy of the legacy one (nothing more), using the same sub-modules and the same tests (API-dependant tests are ran twice: first with the legacy API,then with the yargsa API).
generic and api-specific fixtures are back in a single test/fixtures directory
|
WIP: I still have some linux/ios tests to fix (I'm developping on a windows machine, so these tests are skipped on local runs). I should also add the yargsa API documentation initialization here. |
|
@mleguen I need to give this a through read through, but my first reaction is I don't love the term legacy for the existing API. Yargs is a library used by millions of GitHub repos, tens of thousands of npm repositories, and I personally am not going to be racing to encourage nyc, mocha, or other major consumers to switch to the new API surface; nor do I think it's a good idea to suggest to the community that the API surface will soon be broken. The fact is for 95% of use-cases, the sync API makes writing your application easier; if only because it will be years before top-level I'm open to having an asynchronous export introduced to yargs (I think it's a good idea), and perhaps we could use it to remove some of the asynchronous logic from the existing API -- like you suggested initially (it's weird that that the two paradigms are mixed). In my mind, this would mean that you need to import
We could then strip the half-baked async behavior out of the I think this work is a very worthwhile experiment, but when I think about the longterm health of this project, getting the open issues below 200 is more important to me (in the limited time I have to commit to the project) than a complete overhaul of API surface. I think we should keep this in mind with our goals for async functionality 👌 |
There was a problem hiding this comment.
I'm finding this PR hard to follow, because it changes 8000+ lines of test code. I don't see the point of moving tests into another folder, but if we are going to do so, let's do so in another PR; so that we can keep this PR centered around the specifics of an async API surface for yargs.
| @@ -68,8 +68,8 @@ Retreat from the xupptumblers! | |||
| ```javascript | |||
| #!/usr/bin/env node | |||
| require('yargs') // eslint-disable-line | |||
| .command('serve [port]', 'start the server', (yargs) => { | |||
| yargs | |||
There was a problem hiding this comment.
What's the motivation for shortening this from yargs to y? Were you thinking because the object might be either a yargs or yargsa instance.
If we're going to have the asynchronous API surface in yargs itself, I think I'd prefer that we have something like:
const yargs = require('yargs').async
Similar to require('fs').promises in Node itself, at which point I'd just keep calling the API yargs in samples like this.
| @@ -14,6 +14,8 @@ function Argv (processArgs, cwd) { | |||
| return argv | |||
| } | |||
|
|
|||
| Argv.yargsa = require('./yargsa') | |||
There was a problem hiding this comment.
My thinking is, let's call this Argv.async, at which point, rather than thinking of yargsa as the successor of yargs, we frame it as the import you should use if you're using the async version of the API.
| 'use strict' | ||
| /* global describe */ | ||
|
|
||
| /** |
There was a problem hiding this comment.
As a rule, I usually try to make the directory structure of the tests match the directory structure of the project.
If you would like to make a case for moving tests, I would advocate:
test/lib/completion.js
test/lib/integration.js
test/integration/index.js
test/yargs.js
test/yargs-async.js
However, could we hold off on moving around all the tests until a later date? It makes this pull request much harder to review because it's hard to tell what's new related to the async API, and what is just tests being moved into a new folder.
There was a problem hiding this comment.
I'm finding this PR a little hard to follow, because it changes 8000+ lines of test code.
A refactor like this should be in an unrelated PR, and just move tests around, that way we can keep this PR centered around an async API surface.
I'd advocate starting simple and just adding an test/async.js test, along with a docs/async.md file which describes the async surface; this will help me start to have a better opinion.
|
@bcoe I will give it a 2nd try in a few days to take your remarks into account, and answer your questions. Some of the answers are already in the commit history (I tried to have a commit history as clean as possible to ease the review, knowing a lot of files would be changed). About the (maybe poorly chosen) term of legacy, sorry for misunderstanding your intentions when you first introduced the yargsa name. No problem for me to keep this PR to only dealing with the async issue, as this was my initial intention. |
|
I have to agree this PR is too large. If |
Well, that is exactly what I did here :-) |
|
What I'm suggesting is that a PR that just moves things around without anything else can get approved much easier than a PR that combines large code movement with the actual API changes. |
|
@coreyfarrell OK, I'll do it that way next time. @bcoe I close this PR for now, to let everyone involved have time to think about if and how they really want async support for yargs. I am afraid a do-not-change-too-much approach will not lead us to a better async support than what we already have today, although I completely understand that maintaining support for the huge sync userbase has to be the top priority. |
|
@mleguen I think your idea of a Then, once we have an API we love documented, let's remove the async functionality from the existing API and build out the async module? |
First step of #1487
I recommend merging as soon as it is reviewed and corrected, as it will prevent master and the works on yargsa from drifting too much one from the other.