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

Not compatible with Node v6.0.0 or Node v6.1.0? #530

Closed
hustcer opened this issue May 7, 2016 · 21 comments
Closed

Not compatible with Node v6.0.0 or Node v6.1.0? #530

hustcer opened this issue May 7, 2016 · 21 comments

Comments

@hustcer
Copy link

hustcer commented May 7, 2016

It's strange that when I switch to Node v6.1.0 to execute my app.js -h, just to find the output help content was truncated as:

  Usage: index [options]

  qut is a command line tool for Stock Analysis and Research.

  Options:

    -h, --help             output usage information
    -V, --version          output the version number
    -p, --page   <n>       specify the page index to display.
    -l, --limit  <n>       set total display limit of current page.
    -s, --sort   <sort>    specify field to sort by.
    -f, --format           enable or disable number format.
    -o, --order  <order>   specify order direction: asc or desc.
    -q, --query  <query>   specify collection to query from mongodb. available collections: earning,cal,
                           forecast,fund,gsz,holder,hotspot,insider,investigation,issuance,restrict,
                           shareholder
    -u, --update <update>  update certain datas, available data types: all,quote,cia,range,cal,insider,
                           holder,margin,account,transfer,gsz,forecast,earning,hgt,restrict,price,fprice,
                           dd,fdd,ihustcer@~/github/swift-quant$

When I switch back to Node v5.11.1, it works well:

  Usage: index [options]

  qut is a command line tool for Stock Analysis and Research.

  Options:

    -h, --help             output usage information
    -V, --version          output the version number
    -p, --page   <n>       specify the page index to display.
    -l, --limit  <n>       set total display limit of current page.
    -s, --sort   <sort>    specify field to sort by.
    -f, --format           enable or disable number format.
    -o, --order  <order>   specify order direction: asc or desc.
    -q, --query  <query>   specify collection to query from mongodb. available collections: earning,cal,
                           forecast,fund,gsz,holder,hotspot,insider,investigation,issuance,restrict,
                           shareholder
    -u, --update <update>  update certain datas, available data types: all,quote,cia,range,cal,insider,
                           holder,margin,account,transfer,gsz,forecast,earning,hgt,restrict,price,fprice,
                           dd,fdd,issuance,investigate,shareholder,hotspot.
    -Q, --quant <quant>    filter stocks, available filter types: discount,forecast,earning,insider,gsz,
                           10holder,investigate,shareholder,fund,issuance.
    -S, --stat  <stat>     data statistic, available stat types: insider,insider-week,insider-month,dd,
                           dd-day,dd-week.
    -O, --optimize         filter stocks in optimized way!
    -g, --grep  <kw>       specify the keyword to grep in name or concept, multiple keywords should be
                           separated by ",".

Whether this is a bug or not?

@hustcer
Copy link
Author

hustcer commented May 7, 2016

Node v6.0.0 seems to have the same problem after I update to Node v6.1.0 by homebrew and switch back by nvm use --delete-prefix v6.0.0, however, when I first upgrade to Node v6.0.0, I didn't meet this problem. It's really quite strange.

@nicola
Copy link

nicola commented May 9, 2016

Same issue here, after an x amount of characters, it returns an EOF

@nicola
Copy link

nicola commented May 9, 2016

Apparently --help triggers a unknown command found that triggers a process.exit(1) on subcommands

@zhiyelee
Copy link
Collaborator

zhiyelee commented May 9, 2016

oh, interesting. I will check that, and PR is welcome

@lpinca
Copy link

lpinca commented May 10, 2016

Probably relevant issue: nodejs/node#6456.

@cbetta
Copy link

cbetta commented May 13, 2016

@nicola do you know what the line/lines are that cause this issue?

@nicola
Copy link

nicola commented May 13, 2016

@cbetta nope, but see here: nodejs/node#6456

@cbetta
Copy link

cbetta commented May 14, 2016

@nicola ok thanks. Was just hoping to see if i could make a local patch that fixes it for us by changing the way we exit.

@bcoe
Copy link

bcoe commented May 14, 2016

here's a workaround I'm considering landing in yargs for this problem:

yargs/yargs#501

@bcoe
Copy link

bcoe commented May 14, 2016

@hustcer @cbetta @zhiyelee I pulled the tiny bit of logic that yargs uses to solve this problem into a shim:

https://github.com/yargs/set-blocking

UglifyJS2 uses this same approach, which I modeled yargs' fix after: https://github.com/mishoo/UglifyJS2/pull/1061/files

@cbetta
Copy link

cbetta commented May 16, 2016

@bcoe that's awesome. Would love to see if we can make a patch for commander that uses this fix. What would that take?

@bcoe
Copy link

bcoe commented May 16, 2016

@cbetta yargs writes all of its output in one call, prior to calling process.exit(code);; If commander uses a similar approach, you should be able to do the following:

  1. check whether you are about to call process.exit(0), prior to printing your console output.
  2. if you are about to print the output and call process.exit(0):

set blocking to true:

const setBlocking = require('set-blocking')
setBlocking(true)

output your content like usual:

console.log(commander.generateCLI());

now exit:

process.exit(0);

Note that you need to set blocking to true, prior to printing the console output.

@nicola
Copy link

nicola commented May 16, 2016

@bcoe grat work! Thanks for making it modular!

@cbetta
Copy link

cbetta commented May 29, 2016

@bcoe @nicola sorry not sure i follow. Can I apply this to my CLI program or does this need to be a patch to commander?

@hustcer
Copy link
Author

hustcer commented May 29, 2016

@cbetta
Actually, just add the following lines at the beginning should work:

[process.stdout, process.stderr].forEach(stream => {
  if (stream._handle && typeof stream._handle.setBlocking === 'function') {
    stream._handle.setBlocking(true);
  }
});

@cbetta
Copy link

cbetta commented May 29, 2016

@hustcer my hero! Do you think it would be worth shipping as a module that people can easily include? E.g. blocked-io?

@lpinca
Copy link

lpinca commented May 29, 2016

@cbetta that's what https://github.com/yargs/set-blocking does, also the issue will be fixed soon in core, see nodejs/node#6980.

@cbetta
Copy link

cbetta commented May 29, 2016

@lpinca ah now that earlier comment makes sense. Thanks for clarifying.

@lpinca
Copy link

lpinca commented Jun 9, 2016

I think this can be closed now, It should have been fixed in node 6.2.1.

@zhiyelee
Copy link
Collaborator

zhiyelee commented Jun 9, 2016

thanks @lpinca

@shadowspawn
Copy link
Collaborator

Future readers: this may still be an issue on Windows TTY which are asynchronous according to:

Please open a new issue if you are seeing truncated help.

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

No branches or pull requests

7 participants