Yargs async #1453

mleguen · 2019-10-16T15:33:26Z

See yargs/yargsa#4

bcoe · 2019-10-24T20:51:48Z

@mleguen I'm excited about this work ... it represents a big change to yargs that I didn't expect to have the time to really ever dig into and take on.

It's my goal to set aside a few hours to open-source on Saturday this week, to really dive in and understand this work and your underlying proposal.

One thought, it might be interesting to make an effort to update docs/ and README.md to reflect what this new async world will look like for our existing samples; this will be a good indicator of how disruptive we expect these changes to be.

bcoe

@mleguen this is incredible work; I especially appreciate how in #1452 you document the entire effected API surface.

This API is most likely the future of yargs ... but, I think it might be too disruptive for the next few major versions. Especially since, I think we're in a position where we could use a few stability releases with the parser.

Here's an idea I have, what if we modularize yargs a little bit more, so that there's a ton of shared code, and release a library called yargsa (yargs 'eh). This library would:

expose the async API surface you've drafted in this PR.
be more restrictive with the version of Node.js it supports:
- top-level await is landing soon, and this would make the async yargs API surface much more elegant, I think we should pin to the first Node.js version to introduce this.
- I'd like us to switch to my c8 project for coverage, which becomes much more stable in Node 12+.
It would let us start growing a sub-community of folks playing and testing the async API surface (allowing us to address bugs).

We could link to the async version in the yargs README, and as the community gradually grows, eventually merge the two projects?

What do you think?

test/command.js

mleguen · 2019-11-05T10:30:04Z

One thought, it might be interesting to make an effort to update docs/ and README.md to reflect what this new async world will look like for our existing samples; this will be a good indicator of how disruptive we expect these changes to be.

@bcoe Don't worry: updating docs is in my TODO list, and may be in my mind the most important part of this work...

mleguen · 2019-11-05T10:48:13Z

@bcoe Regarding your suggestion of yargsa:

pros: it would indeed temper the potentially disruptive effect of this new API
cons: as most of the code is impacted, this would mean, in practice, either maintaining 2 branches of code (yargs and yargsa), or deprecating yargs as is and maintaining only yargsa

mleguen · 2019-11-05T10:48:41Z

@bcoe I do not see however how we could benefit from top level await, as we have no dynamic initializing in Yargs. We would export an async API, but we would export it statically, so there would be no need for us to use top level await.

EDIT: could you detail a use case for top level await in some of yargs module?

mleguen · 2019-11-05T10:57:16Z

@bcoe about c8: If I understand well, this is a replacement for nyc, using builtin node functionality? This looks exciting!

However, the drawback would be the need for node v12, meaning we would have to support both yargs and yargsa at least for 18 months (until node v10 is out of maintenance). Would we have enough time for that?

EDIT: I see that your already switched yargs to c8, testing only coverage for node v13, which makes sense as we do not need to test coverage for all versions we support (this is only a dev indicator). So ignore my "drawback" above).

bcoe · 2019-11-05T20:11:17Z

@mleguen I'm empathetic to the drawbacks of having an async version of yargs in conjunction with a sync version, however, some of the biggest users of yargs require that its behavior is sync (nyc, mocha, etc).

And I think that the disruption to the existing community is (at this time) not worth the benefit we'd get from an async API. As you've mentioned yourself, the async features we have are a bit odd, and not super compelling:

middleware.
handler responses.
completions.

However, as we Node.js/JavaScript continues to evolve, and we get top-level await, async-iterators, and other delightful abstractions for async code, I think that there will start to be a reasonable case for yargs being async.

I don't think it would be worth making this jump for the next few majors though, and we'd provide more value to the community continuing to address some of the backlog of bugs, oddities, etc. This makes me think if we'd like to start experimenting with an async API, it would at least be worth considering floating two versions of yargs at once.

bcoe · 2019-11-10T00:14:23Z

@mleguen I've created https://github.com/yargs/yargsa, which you are a maintainer of, I've also published https://www.npmjs.com/package/yargsa (as a placeholder for our work).

I'd like to advocate that we pull together an async version of yargs in this repo, I think it would be a great opportunity to play with:

async/await (and top level await as soon as it becomes available).
ESM modules (I'd advocate we potentially try to write this library using ESM modules from the get go, as long as we're experimenting with future APIs.

Let's use this as a playground to flesh out the version of yargs we'd like to see in the future.

mleguen · 2019-11-13T07:27:09Z

@bcoe Thank you!

And Wow! for your hard work of the two past weeks on yargs!

I'd advocate we potentially try to write this library using ESM modules

Why not switching to typescript?

we could output ESM modules (and other formats as well if needed)
we would benefit from typings in our devs
our typescript users would have accurate typings without having to rely on potentially outdated @types/yargs
our non typescript users would not be impacted
etc.

At the moment, Yargs is the only project I am still developping in plain JS

4 broken tests are skipped, to be fixed in following commits

…s async Allows 3 of the 4 broken tests to be OK back

The last broken test in completion is back online

1 skipped broken test, to be fixed in following commits

Bring the broken test in middeware back online

25 broken tests, to be fixed in following commits

Fix 25 broken tests expecting parse() to return a result after calling process.exit, which was only a side effect of old checkUsage() not to occur in real life

It is more readable to explicitely write that a rejection is expected instead of ignoring the rejection when it occurs. Moreover, the corresponding tests now fail when there is no rejection, which is what is expected.

To keep the current behavior (not calling parseFn when _parseArgs throws)

done() is called before should.be.rejected (and not sould) is evaluated. Moreover, it should not be rejected as there is no rejection when there is a fail callback

2 broken tests, to be fixed in following commits

Fix 1 broken test expecting parse() to return a result after calling process.exit, which was only a side effect of old checkUsage() not to occur in real life

expect.fail() should not be used in a test using a done callback: - if it covers a path which should not be reached, done(err) should be used instead - it has no use if called after done(): not calling done is enough for the test not to be successful Bring the last broken test for validation back online.

2 broken tests, to be fixed in following commits

No longer needed.

to align its async behavior with completion function

The test did not really tested the handler was called, as the promise was already created beforeHand. Moreover, now also tests the handler completes before await parse() returns.

To align its async behavior with completion and builder functions.

To align its async behavior with other async allowed callbacks.

mleguen · 2019-11-13T18:04:12Z

I also aligned the async behavior of all async allowed callbacks (command builder and handlers, middlewares and completion function).

They can now all be:

either a sync function
or an async function returning a promise
or an async function calling a done callback

mleguen · 2019-11-13T18:06:30Z

@bcoe @cspotcode, @petrgrishin, @Goodluckhf ready for review

lib/command.js

lib/middleware.js

cspotcode · 2019-11-14T00:33:52Z

I'd advocate we potentially try to write this library using ESM modules

Why not switching to typescript?

@mleguen @bcoe Yes, the library should be written in TypeScript. As is, the code is unnecessarily annoying to review because it's not clear the type of values passing through the codebase.

lib/middleware.js

bcoe · 2019-11-15T00:16:02Z

@cspotcode @mleguen I'm open to TypeScript.

All async callbacks (command builders and handlers, middlewares and completion functions) can now either return a promise or call a standard js (err, value) callback. Corresponding tests reviewed and added when missing.

1 test is broken as a global middleware failure leads to fail being called if the middleware runs after validation, not before, which is incoherent and should be fixed

cspotcode · 2019-11-15T19:57:03Z

@bcoe @mleguen Awesome, feel free to ping me on anything related to a TS port.

I took a stab at it a while back, so I went ahead and pushed my branch and filed a PR. #1478

bcoe · 2019-11-15T20:14:00Z

@cspotcode @mleguen would you be open to closing this PR, and moving the pertinent conversation we've had to a conversation on yargsa?

cspotcode · 2019-11-15T20:33:20Z

Yeah, works for me.

…

On Fri, Nov 15, 2019, 3:14 PM Benjamin E. Coe ***@***.***> wrote: @cspotcode <https://github.com/cspotcode> @mleguen <https://github.com/mleguen> would you be open to closing this PR, and moving the pertinent conversation we've had to a conversation on yargsa? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1453?email_source=notifications&email_token=AAC35ODKBJTQU4HMLU2527DQT37IZA5CNFSM4JBNJN22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEGSWZI#issuecomment-554511205>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAC35ODF3CUQCVEDA4ZB7HDQT37IZANCNFSM4JBNJN2Q> .

bcoe · 2019-11-16T22:28:02Z

Conversation moved here: https://github.com/yargs/yargsa/issues/4

@mleguen do you think you'd be open to making an attempt at PR on yargsa? If we want to start simple, it could literally be a fork of yargs, but with your async work from this branch ...

if you, @cspotcode, and others have the will and the cycles, we could also try an approach that's slightly more from the ground up.

OJezu · 2019-11-28T13:42:29Z

yargs/yargsa repository does not seem to be public, Github returns 404 for me.

tamj0rd2 · 2020-09-04T16:09:52Z

Did anything else happen with yargsa?

cspotcode · 2020-09-04T17:28:58Z

If I recall correctly, nothing much happened with yargsa. If I recall correctly, there was disagreement on the name of a function in the API surface, and the code being JS instead of TS was making it unnecessarily time-consuming to reason about any refactors.

Someone was working on porting yargs to TS, which seems to be mostly complete. They said they no longer have time to lead that effort. I'm not sure what still remains to be done.

tamj0rd2 · 2020-09-04T17:30:57Z

Someone was working on porting yargs to TS, which seems to be mostly complete. They said they no longer have time to lead that effort. I'm not sure what still remains to be done.

Do you know if that PR or branch still exists somewhere? I'd be happy to contribute to it

Edit: Ah, I'm guessing this is probably the one: #1586

cspotcode · 2020-09-04T17:38:46Z

Yeah, I guess that's it. Seems like steps 5-9 are pretty straightforward. @types/yargs should already have tests which can be used to verify the bundled declarations. Contributions to @types get merged within a week, so that can happen quickly. I don't know if any work has been done converting yargs-parser to TS. I wonder what ts-migrate would do with it?

bcoe reviewed Oct 26, 2019

View reviewed changes

test/command.js Show resolved Hide resolved

bcoe mentioned this pull request Oct 26, 2019

WIP: feature(parse): sync/async API #1443

Closed

mleguen force-pushed the feature/1452 branch 2 times, most recently from 57af615 to a039f20 Compare November 6, 2019 15:04

bcoe mentioned this pull request Nov 9, 2019

Allow command handler to accept a promise #510

Closed

Mael LE GUEN added 17 commits November 13, 2019 10:38

feature: await command handlers

e8fc861

tests(command): await command handlers impacts

08ade38

tests(completion): await command handlers impacts

ebca6db

4 broken tests are skipped, to be fixed in following commits

feature(completion): remove done callback from getCompletion now it i…

924f8ee

…s async Allows 3 of the 4 broken tests to be OK back

fix(completion): add forgotten async handling

6b4fe94

The last broken test in completion is back online

tests(integration): await command handlers impacts

bdc4d85

tests(middleware): await comand handlers impacts

bd325be

1 skipped broken test, to be fixed in following commits

fix(middleware): call fail when middleware rejects

d92db5e

Bring the broken test in middeware back online

tests(usage): await command handlers impacts

83b6f2d

25 broken tests, to be fixed in following commits

tests(usage): fix incorrect tests

6bcd92f

Fix 25 broken tests expecting parse() to return a result after calling process.exit, which was only a side effect of old checkUsage() not to occur in real life

chore(tests): replace promisifyTest(, true) by should.be.rejected

1038ee9

It is more readable to explicitely write that a rejection is expected instead of ignoring the rejection when it occurs. Moreover, the corresponding tests now fail when there is no rejection, which is what is expected.

fix(yargs): do not call parseFn on unexpected rejections

034df2f

To keep the current behavior (not calling parseFn when _parseArgs throws)

test(middleware): remove never reached expectation

b451c22

done() is called before should.be.rejected (and not sould) is evaluated. Moreover, it should not be rejected as there is no rejection when there is a fail callback

test(validation): await command handlers impacts

6d2ca10

2 broken tests, to be fixed in following commits

tests(validation): fix incorrect test

1b854be

Fix 1 broken test expecting parse() to return a result after calling process.exit, which was only a side effect of old checkUsage() not to occur in real life

test(yargs): await command handlers impacts

c827966

2 broken tests, to be fixed in following commits

Mael LE GUEN added 4 commits November 13, 2019 17:53

feature(usage): remove cacheHelpMessage

9168f11

No longer needed.

chore(command): group builder function tests

9c5a06f

feature(command): allow builder function to call a done callback

6f94dd3

to align its async behavior with completion function

fix(test): fix an old async command test

2229d84

The test did not really tested the handler was called, as the promise was already created beforeHand. Moreover, now also tests the handler completes before await parse() returns.

mleguen force-pushed the feature/1452 branch from bbcbbb4 to f837e57 Compare November 13, 2019 17:48

feature(command): allow handler function to call a done callback

43122d0

To align its async behavior with completion and builder functions.

mleguen force-pushed the feature/1452 branch from f837e57 to 43122d0 Compare November 13, 2019 17:49

feature(command): allow middleware function to call a done callback

cc3c9d8

To align its async behavior with other async allowed callbacks.

mleguen marked this pull request as ready for review November 13, 2019 18:04

Goodluckhf suggested changes Nov 13, 2019

View reviewed changes

lib/command.js Outdated Show resolved Hide resolved

lib/command.js Outdated Show resolved Hide resolved

lib/middleware.js Outdated Show resolved Hide resolved

mleguen commented Nov 14, 2019

View reviewed changes

lib/middleware.js Outdated Show resolved Hide resolved

chore(middleware): remove forgotten TODO

eadb3f7

Mael LE GUEN added 2 commits November 15, 2019 11:00

test: new sleep helper function for async tests

e542e88

feature(async): standardize async callbacks

155a5d9

All async callbacks (command builders and handlers, middlewares and completion functions) can now either return a promise or call a standard js (err, value) callback. Corresponding tests reviewed and added when missing.

mleguen force-pushed the feature/1452 branch from f05b123 to 155a5d9 Compare November 15, 2019 11:10

test(middleware): add chaining tests

049d307

1 test is broken as a global middleware failure leads to fail being called if the middleware runs after validation, not before, which is incoherent and should be fixed

bcoe closed this Nov 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Yargs async #1453

Yargs async #1453

mleguen commented Oct 16, 2019 •

edited

bcoe commented Oct 24, 2019

bcoe left a comment •

edited

mleguen commented Nov 5, 2019

mleguen commented Nov 5, 2019

mleguen commented Nov 5, 2019 •

edited

mleguen commented Nov 5, 2019 •

edited

bcoe commented Nov 5, 2019

bcoe commented Nov 10, 2019

mleguen commented Nov 13, 2019

mleguen commented Nov 13, 2019

mleguen commented Nov 13, 2019

cspotcode commented Nov 14, 2019

bcoe commented Nov 15, 2019

cspotcode commented Nov 15, 2019

bcoe commented Nov 15, 2019

cspotcode commented Nov 15, 2019 via email

bcoe commented Nov 16, 2019

OJezu commented Nov 28, 2019

tamj0rd2 commented Sep 4, 2020

cspotcode commented Sep 4, 2020

tamj0rd2 commented Sep 4, 2020 •

edited

cspotcode commented Sep 4, 2020 •

edited

Yargs async #1453

Yargs async #1453

Conversation

mleguen commented Oct 16, 2019 • edited

bcoe commented Oct 24, 2019

bcoe left a comment • edited

Choose a reason for hiding this comment

mleguen commented Nov 5, 2019

mleguen commented Nov 5, 2019

mleguen commented Nov 5, 2019 • edited

mleguen commented Nov 5, 2019 • edited

bcoe commented Nov 5, 2019

bcoe commented Nov 10, 2019

mleguen commented Nov 13, 2019

mleguen commented Nov 13, 2019

mleguen commented Nov 13, 2019

cspotcode commented Nov 14, 2019

bcoe commented Nov 15, 2019

cspotcode commented Nov 15, 2019

bcoe commented Nov 15, 2019

cspotcode commented Nov 15, 2019 via email

bcoe commented Nov 16, 2019

OJezu commented Nov 28, 2019

tamj0rd2 commented Sep 4, 2020

cspotcode commented Sep 4, 2020

tamj0rd2 commented Sep 4, 2020 • edited

cspotcode commented Sep 4, 2020 • edited

mleguen commented Oct 16, 2019 •

edited

bcoe left a comment •

edited

mleguen commented Nov 5, 2019 •

edited

mleguen commented Nov 5, 2019 •

edited

tamj0rd2 commented Sep 4, 2020 •

edited

cspotcode commented Sep 4, 2020 •

edited