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

Revert tty behavior for inquirer 7.x #903

Merged
merged 3 commits into from
Mar 10, 2020

Conversation

mshima
Copy link
Contributor

@mshima mshima commented Mar 9, 2020

  • Move tty check to setupReadlineOptions
  • Add skipTTYChecks
  • Enable skipTTYChecks by default for 7.x behavior stability.

Fixes #902.

@mshima mshima force-pushed the fix_yeoman_test branch 3 times, most recently from 4b8e3f6 to 9cfb4cb Compare March 9, 2020 18:41
@mshima mshima force-pushed the fix_yeoman_test branch from 9cfb4cb to 02bc69d Compare March 9, 2020 18:46
@codecov
Copy link

codecov bot commented Mar 9, 2020

Codecov Report

Merging #903 into master will decrease coverage by 0.26%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #903      +/-   ##
==========================================
- Coverage   93.53%   93.27%   -0.27%     
==========================================
  Files          26       26              
  Lines        1068     1071       +3     
  Branches       23       23              
==========================================
  Hits          999      999              
- Misses         69       72       +3
Impacted Files Coverage Δ
packages/inquirer/lib/ui/prompt.js 100% <ø> (ø) ⬆️
packages/inquirer/lib/ui/baseUI.js 90.9% <100%> (-9.1%) ⬇️
packages/inquirer/lib/inquirer.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c521b7...3b86d23. Read the comment docs.

Copy link
Contributor

@ruyadorno ruyadorno left a comment

Choose a reason for hiding this comment

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

I would love if you could port the test that uses a valid manually-opened tty from #898 into this - ensuring there's no regressions even if folks were to use skipTTYChecks: false:

    it('No exception when using tty other than process.stdin', function() {
      // Manually opens a new tty
      var input = new tty.ReadStream(fs.openSync('/dev/tty', 'r+'));

      // Uses manually opened tty as input instead of process.stdin
      var prompt = inquirer.createPromptModule({
        input: input,
        skipTTYChecks: false
      });

      var prompts = [
        {
          type: 'input',
          name: 'q1',
          default: 'foo',
          message: 'message'
        }
      ];

      var promise = prompt(prompts);
      promise.ui.rl.emit('line');

      // Release the input tty socket
      input.unref();

      return promise.then(answers => {
        expect(answers).to.deep.equal({ q1: 'foo' });
      });
    });

it will also help with getting a higher coverage

packages/inquirer/lib/ui/baseUI.js Show resolved Hide resolved
@ruyadorno
Copy link
Contributor

@mshima properly sent you the test here: mshima#1

@SBoudrias
Copy link
Owner

Thanks to both of you for the help on this issue :D

@SBoudrias SBoudrias merged commit b4b01d5 into SBoudrias:master Mar 10, 2020
@SBoudrias
Copy link
Owner

7.0.7 is out now.

jdoyle65 pushed a commit to jdoyle65/Inquirer.js that referenced this pull request Jan 19, 2021
* Move tty checks back to setupReadlineOptions

* Implement skipTTYChecks and keep it enabled by default for inquirer 7.x.

* test: add manual open tty check

Co-authored-by: Ruy Adorno <ruyadorno@hotmail.com>
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.

Produce error on prompt in non-tty environment.
3 participants