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

Async typescript functions as actions #806

Closed
sr258 opened this issue May 21, 2018 · 14 comments
Closed

Async typescript functions as actions #806

sr258 opened this issue May 21, 2018 · 14 comments

Comments

@sr258
Copy link

sr258 commented May 21, 2018

When I pass an async typescript function into a command's action() method, the async function is called correctly, but is not run to the end, as the program does not wait when awaiting another function. It simply ends execution. I've seen a similar question here, but it was not answered, as far as I understand.

Consider this example:

#!usr/bin/env node

import * as commander from 'commander';
import axios from 'axios';

async function run() : Promise<void>  {  
    var response = await axios.get("https://dummyimage.com/600x400/000/fff", { "responseType": "arraybuffer" });
    if (response.status === 200) {
        console.log(`Downloaded file: ${response.data.byteLength} bytes.`)
    }
    else {
        console.error('Error downloading');        
    } 
}

async function main() {
  commander
  .command("do", "Test")
  .action(run)
  .parse(process.argv);
//  await run();
  console.log('finished');
}

try {
  main();
}
catch (error) {
  console.error(error);
}

If you run the example above, you can see that finished is called but the download is never finished. Calling await run() without the help of commander works, though.

So, is it possible to use commander with typescript's async functions?

@sr258

This comment has been minimized.

@bigtfromaz
Copy link

bigtfromaz commented Aug 12, 2018

I investigated yargs as an option but frankly I think commander.js offer the best design. However that doesn't mean that commander.js does not have an issue in this regard.

I am very experienced but quite new to JavaScript. It leaves a lot to be desired when compared to most other languages.

With that said, I find myself in a position where I have to build an enterprise-quality command-line app that must run under node.js. The app will also offer a REST API interface to some functions. Obviously neither it, nor the CLI can be allowed to block the message/UI thread.

commander.js seems to be a little schizophrenic in this regard. Is it a parser or a command dispatcher? It's a really nice parser. Dispatcher? No so much. Command actions definitely do not block the thread however there is no reasonable way to return anything from the handling function, including a Promise.

I have evaluated a few options but none of them will allow me to sleep well at night after this thing goes live. So I have a suggested change:

  1. Offer a version of command.parse that doesn't actually dispatch an action but rather, returns the objects that would have been supplied when calling the action. This would allow the consuming app to dispatch it's own method using either Promise or TypeScript async/await.

  2. Offer awaitable and sync versions of command.parse that returns whatever value is returned by the action function.

I would suggest option 1 as it provides a clean separation between parsing and functional execution.

I thought about forking this project and making the changes myself but I have to say my learning curve would make it too costly and that same learning curve would probably produce less than optimal source code. If you were not able to accept our pull request then we would be in the situation where we would have to merge your changes into our version, forever.

This is just food for thought. For our project we may write a parser from the ground up using TypeScript and the semantics of commander.js in the spec. Another possibility will be to just write a parser specific to the apps requirements.

By the way, if there is something I have wrong, please feel free to constructively enlighten me.

@chigix
Copy link

chigix commented Oct 9, 2018

I think this behavior is correct. Exactly, promise-like return is not provided through commander api though.

Why not wrap commander with promise api manually? By the way, I also don't think that it is needed to do jobs simultaneously around commander initiation. Generally, commander initiation should be down at the top level of a problem or a system at the entry file, which, I think, is a best practice.

@alexx666
Copy link

alexx666 commented Nov 2, 2018

I had the same issue, turned out it was a version problem.
Upgrading from version 2.15.0 to 2.18.0 fixed the issue.

@chrisgo
Copy link

chrisgo commented Feb 27, 2019

Can this pattern be used?

https://stackoverflow.com/questions/48376479/executing-multiple-sequelize-js-model-query-methods-with-promises-node?answertab=oldest#tab-top

const axiosPromise = axios.get(...);
const sequelizePromise = sequelize.query(...);
const anotherPromise = anotherFunctionThatReturnsAPromise();
Promise
    .all([axiosPromise, sequelizePromise, anotherPromise])
    .then(responses => {
        console.log('**********COMPLETE RESULTS****************');
        console.log(responses[0]); // axios result
        console.log(responses[1]); // sequelize result
        console.log(responses[2]); // another result
    })
    .catch(err => {
        console.log('**********ERROR RESULT****************');
        console.log(err);
    });

@sr258
Copy link
Author

sr258 commented Feb 28, 2019

@chrisgo I'm sure it can, but the point of my question was if it is possible to use async await.

@victor-shelepen
Copy link

I had the same issue, turned out it was a version problem.
Upgrading from version 2.15.0 to 2.18.0 fixed the issue.
I have 2.19.0 The async actions are not managed well.

@victor-shelepen
Copy link

I looked in the code. I see it is not prepared for async chaining at all. EventEmmiter is used to execute a command. Is a strategy pattern enough? Do contributors have plans to adjust it to be chainable?

@shadowspawn
Copy link
Collaborator

I came across this issue on Yargs to improve their async support. Some interesting detail on possible code patterns: yargs/yargs#1420

@jdmarshall
Copy link

jdmarshall commented Oct 24, 2019

Same problem.

Oddly, some async tasks run to completion. Others do not. In particular, waiting on a promise that never resolves exits with status 0, which is a big no-no. version 3.0.2

@jdmarshall
Copy link

jdmarshall commented Oct 24, 2019

Cheese it:

    let lifeline = setInterval(() => {}, 500);
    try {
        ...
    } finally {
        clearInterval(lifeline);
    }

@shadowspawn
Copy link
Collaborator

I am wondering about option 2 from #806 (comment)

So, we would add a .parseAsync() which you should use instead of .parse() if you have any async action handlers, and it returns whatever the action handler returns. Up to calling code to call the correct parse and do the right thing with what it returns.

Might that work for people wanting to use async?

@shadowspawn
Copy link
Collaborator

I have added a draft Pull Request to add .parseAsync(). Feedback from interested parties welcome.

@shadowspawn
Copy link
Collaborator

shadowspawn commented Jan 6, 2020

Added .parseAsync() in Commander v4.1.

See https://github.com/tj/commander.js#action-handler-subcommands

@shadowspawn shadowspawn removed their assignment Jan 6, 2020
prokopsimek added a commit to prokopsimek/commander.js that referenced this issue Jan 29, 2020
Since we can use `parseAsync` for async functions so the action can be asynchronous.

related to tj#806
prokopsimek added a commit to prokopsimek/commander.js that referenced this issue Jan 29, 2020
Since we can use `parseAsync` for async functions so the action can be asynchronous.

related to tj#806
prokopsimek added a commit to prokopsimek/commander.js that referenced this issue Jan 30, 2020
Since we can use `parseAsync` for async functions so the action can be asynchronous.

related to tj#806
prokopsimek added a commit to prokopsimek/commander.js that referenced this issue Jan 30, 2020
Since we can use `parseAsync` for async functions so the action can be asynchronous.

related to tj#806
prokopsimek added a commit to prokopsimek/commander.js that referenced this issue Jan 30, 2020
Since we can use `parseAsync` for async functions so the action can be asynchronous.

related to tj#806
shadowspawn pushed a commit that referenced this issue Feb 2, 2020
Since we can use `parseAsync` for async functions so the action can be asynchronous.

related to #806
bestlucky0825 pushed a commit to bestlucky0825/commander.js that referenced this issue Jun 1, 2022
Since we can use `parseAsync` for async functions so the action can be asynchronous.

related to tj/commander.js#806
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants