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

Feature/async parse args experiment #1913

Closed

Conversation

shadowspawn
Copy link
Collaborator

@shadowspawn shadowspawn commented Jul 25, 2023

This is an experiment based on concepts in #1902 to support async Option.parseArgs and Arguments.parseArgs. Blindly store the return value from async calls to parseArgs as usual (although actually a pending promise), and then settle the promises in a batch before proceeding to next stage of parsing. In particular, settle the promises before calling user hooks or action handler so client code is unlikely to see the promises.

Simplified approach compared to #1902, here:

  • no configuration
  • built-in support rather than using hooks
  • handling previous value promises from repeated calls to parseArgs left to client

Reminder to add co-author credit if proceed with this PR.

Co-authored-by: Wee Bit <aweebit64@gmail.com>

@aweebit
Copy link
Contributor

aweebit commented Jul 25, 2023

What about checking for whether parseAsync() has been called? Don't we want to leave the option for the user to handle promises returned from parsers manually at least when parse() is used like in my original example in #1900, and thus make the change non-breaking at least for parse() calls?

@shadowspawn
Copy link
Collaborator Author

Good question. From that point of view, either the change is breaking or non-breaking. It does not matter whether parseAsync has been called.

One way of shipping it in a minor version would be to make it opt-in, before making it always-on in the next major version.

A possible fallback is to just add an example file for now! It all looked nice and easy for limited use case like: #1900 (comment)

@aweebit
Copy link
Contributor

aweebit commented Jul 25, 2023

Yeah, I know it is going to be breaking in the end anyway, I just meant that by checking for parseAsync(), we could ensure nothing changes from users' perspective at least when parse() is called. After all, a user might want to handle promises returned from parsers manually, but by awaiting them even if parse() is used, we not only deprive them of this option, but also introduce the risk of unhandled promise rejections.

@aweebit
Copy link
Contributor

aweebit commented Jul 25, 2023

I have submitted a new PR (#1915) which gets rid of all the complexity due to hook usage in #1902 but keeps all the parts missing in your solution that I think are necessary (the motivation is explained in detail in the new PR's description).

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Jul 26, 2023

Added support for multiple calls to parseArgs to resolve previous value (known as chainArgParserCalls in #1915). This is what the user would probably expect, and I mainly left it out until now because it is tricky to implement!

@shadowspawn
Copy link
Collaborator Author

After all, a user might want to handle promises returned from parsers manually, ...

I think we probably only need to support built-in async. If the user wants some variant of the promise resolving they can move the async processing to a hook, so not blocked. Less clever but less magic too.

lib/command.js Outdated
}
}

if (thenable(result)) result.catch(refineError);
Copy link
Contributor

Choose a reason for hiding this comment

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

refineError() throws, so there will be an unhandled promise rejection. The result of the catch() call needs to be assigned to result to prevent it.

But if previous is already thenable, it is better to call catch() in the previous.then() callback (and return the result). Otherwise, catch() might be called on an already failed promise chain in which an error has already been caught and rethrown (because that is what refineError() does), so refineError() would be called again which does not make sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I think calling result.then(x => x, refineError) instead of result.catch(refineError) is more robust.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The result of the catch() call needs to be assigned to result

Oops, yes, good catch.

Otherwise, catch() might be called on an already failed promise chain in which an error has already been caught and rethrown

I think this is correct pattern, but I'll try it before explaining my reasoning.

Also, I think calling result.then(x => x, refineError) instead of result.catch(refineError) is more robust.

Because we only test for .then()? Yes, I have thought about that in past and then forgot. I want to just assume they are promise-like since the primary use-case is for async, but it isn't hard to only rely on then and cover a few more edge uses.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But if previous is already thenable, it is better to call catch() in the previous.then() callback (and return the result). Otherwise, catch() might be called on an already failed promise chain in which an error has already been caught and rethrown (because that is what refineError() does), so refineError() would be called again which does not make sense.

Took me a while to understand this. I was expecting a long catch chain, but see it can be avoided. 👍

@aweebit
Copy link
Contributor

aweebit commented Jul 26, 2023

Added support for multiple calls to parseArgs to resolve previous value (known as chainArgParserCalls in #1915). This is what the user would probably expect, and I mainly left it out until now because it is tricky to implement!

But there are use cases where the user might not want that, so making chaining configurable now suggests itself. That is why I kept chainArgParserCalls() in #1915.

@aweebit
Copy link
Contributor

aweebit commented Jul 26, 2023

I think we probably only need to support built-in async. If the user wants some variant of the promise resolving they can move the async processing to a hook, so not blocked. Less clever but less magic too.

But all currently available hooks are only called after the available thenable option and argument values are resolved, so this does not seem like a good option to me. The user would have no access to the promises returned by async parsers at all, even when calling parse().

In its simplest non-magical form (independent from whether parse() or parseAsync() was called and inherited via copyInheritedSettings()), awaiting configuration requires 10 lines of code which I don't think is such a big deal.

  • 1 line for property initialization in constructor
  • 1 line for property inheritance in copyInheritedSettings()
  • 4 lines for configuration function (await() in #1915)
  • 2 lines for if statements in each of _settleOptionPromises() and _settleArgumentPromises()

But again, an explanation why a little "magic" is meaningful is given in #1915, with additional details in this comment.

@shadowspawn
Copy link
Collaborator Author

But all currently available hooks are only called after the available thenable option and argument values are resolved, so this does not seem like a good option to me. The user would have no access to the promises returned by async parsers at all, even when calling parse().

Yes, the work-around I was suggesting for what are hopefully rare cases was use synchronous parseArg.

I understand this is not the flexibility you have proposed.

@aweebit
Copy link
Contributor

aweebit commented Jul 28, 2023

Yes, the work-around I was suggesting for what are hopefully rare cases was use synchronous parseArg.

Did not get it back then, but I see what you mean now 👍

aweebit added a commit to aweebit/commander.js that referenced this pull request Jul 28, 2023
@shadowspawn
Copy link
Collaborator Author

This is pretty cool, but I am not happy enough with it:

  • uses an indirect approach to achieve async with reprocessing options (and args), not direct support for async functions
  • not a nice place to slot in async for program which has neither action handler nor external command
  • would add complexity to one of the longer routines and somewhat fragile routines

User can achieve most of the functionality themselves fairly easily because they know how their program is constructed.

I am currently thinking async action handlers would be a neat example, but not a great built-in feature.

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.

None yet

2 participants