tj / commander.js

node.js command-line interfaces made easy
MIT License
26.77k stars 1.69k forks source link

Allow method _chainOrCall to return the promise result. #1907

Closed Rmannn closed 1 year ago

Rmannn commented 1 year ago

Hi, I am the developper of a nestjs module using commander nestjs-console Our module is built to partially wrap your module logic into a nestjs module.

We were extracting response from the command until the new version.

https://github.com/tj/commander.js/blob/4ef19faac1564743d8c7e3ce89ef8d190e1551b4/lib/command.js#L1301-L1305 As you can see here, actionResult will always returns undefined.

Can we add a line to return the result ?

if (this.parent) { 
   actionResult = this._chainOrCall(actionResult, () => { 
     this.parent.emit(commandEvent, operands, unknown); // legacy 
     // new line to allow us extracting the response
     return actionResult
   }); 
 } 

I could help opening a PR if you want.

Thx

shadowspawn commented 1 year ago

I might be misunderstanding your question, so ask again if this answer does not make sense.

Commander does not expect the action handler to return a response, other than a promise if it is async. You might well think it does based on the actionResult variable name, but that it purely for supporting promises. The actionResult is used to work out whether to chain the next piece of code or to call it directly. A non-promise return value from the action handler is promptly forgotten in the next call to _chainOrCall.

For interest, you can see the action handler response was ignored before support for async was added. This is the PR adding the first version of async support: https://github.com/tj/commander.js/pull/1118/files

shadowspawn commented 1 year ago

The link to #1118 was more relevant than I expected, it looks like you were relying on that implementation to indirectly get the action response, although _actionResults was only being retained to settle promises.

https://github.com/Pop-Code/nestjs-console/pull/345/files#diff-84b68bce2abfcb25c3328df0286963d7dacc924940adc41923bf4f502a6bc95aR105

Again, the internal variable name could lead to incorrect assumptions. Sorry about that. But the public facing API has the action handler returning void: https://github.com/tj/commander.js/blob/4ef19faac1564743d8c7e3ce89ef8d190e1551b4/typings/index.d.ts#L497

Rmannn commented 1 year ago

Thanks for your response.