Skip to content
This repository has been archived by the owner on Feb 1, 2022. It is now read-only.

fix: pause prompt stdin #36

Merged
merged 7 commits into from
Jun 12, 2018
Merged

fix: pause prompt stdin #36

merged 7 commits into from
Jun 12, 2018

Conversation

RasPhilCo
Copy link
Contributor

No description provided.

@RasPhilCo RasPhilCo requested a review from jdx June 1, 2018 18:57
src/prompt.ts Outdated
setTimeout(() => reject(), options.timeout || 10000).unref()
setTimeout(() => {
process.stdin.end()
reject(new Error('Prompt timeout'))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not having an error object in reject was causing issues later in oclif/config

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel that's something TypeScript should enforce (possibly behind a flag)

src/prompt.ts Outdated
@@ -81,6 +81,9 @@ function normal(options: IPromptConfig, retries = 100): Promise<string> {
resolve(data || options.default)
}
})
setTimeout(() => reject(), options.timeout || 10000).unref()
setTimeout(() => {
process.stdin.end()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

missing piece. 🍕

Copy link
Contributor

Choose a reason for hiding this comment

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

this would prevent us from reading stdin ever again in this process which seems problematic

Copy link
Contributor

Choose a reason for hiding this comment

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

would process.stdin.pause() work?

@codecov
Copy link

codecov bot commented Jun 1, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@c1f6cff). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #36   +/-   ##
=========================================
  Coverage          ?   46.26%           
=========================================
  Files             ?       13           
  Lines             ?      335           
  Branches          ?       72           
=========================================
  Hits              ?      155           
  Misses            ?      153           
  Partials          ?       27
Impacted Files Coverage Δ
src/prompt.ts 65.85% <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 c1f6cff...da2525a. Read the comment docs.

src/prompt.ts Outdated
@@ -81,8 +81,9 @@ function normal(options: IPromptConfig, retries = 100): Promise<string> {
resolve(data || options.default)
}
})
if (options.timeout) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason you removed this conditional?

Copy link
Contributor

Choose a reason for hiding this comment

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

I put it in to fix this issue: heroku/cli#880

Copy link
Contributor Author

Choose a reason for hiding this comment

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

merge mistake, putting it back

@RasPhilCo RasPhilCo changed the title fix: end prompt stdin fix: pause prompt stdin Jun 12, 2018
src/prompt.ts Outdated
setTimeout(() => {
process.stdin.pause()
reject(new Error('Prompt timeout'))
}, options.timeout || 10000).unref()
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to default with the conditional in place

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol

@RasPhilCo RasPhilCo merged commit 1443998 into master Jun 12, 2018
@RasPhilCo RasPhilCo deleted the unref-stdin branch June 12, 2018 23:45
oclif-bot added a commit that referenced this pull request Jun 12, 2018
<a name="4.6.3"></a>
## [4.6.3](v4.6.2...v4.6.3) (2018-06-12)

### Bug Fixes

* pause prompt stdin ([#36](#36)) ([1443998](1443998))
@oclif-bot
Copy link
Contributor

🎉 This PR is included in version 4.6.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants