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

Add cursorTo & clearLine to streams #7082

Closed
wants to merge 1 commit into from

Conversation

reyronald
Copy link
Contributor

Using process.stdout.cursorTo and process.stdout.clearLine will error in Flow because those definitions are missing.

Error: src/db/seeds/utils/helpers.js:9
  9:   process.stdout.cursorTo(0)
                      ^^^^^^^^ property `cursorTo`. Property not found in
  9:   process.stdout.cursorTo(0)
       ^^^^^^^^^^^^^^ stream$Writable

Error: src/db/seeds/utils/helpers.js:9
  9:   process.stdout.cursorTo(0)
                      ^^^^^^^^ property `cursorTo`. Property not found in
  9:   process.stdout.cursorTo(0)
       ^^^^^^^^^^^^^^ tty$WriteStream

node/lib/readline.js
node/lib/tty.js

Error: src/db/seeds/utils/helpers.js:10
 10:   process.stdout.clearLine()
                      ^^^^^^^^^ property `clearLine`. Property not found in
 10:   process.stdout.clearLine()
       ^^^^^^^^^^^^^^ stream$Writable

Error: src/db/seeds/utils/helpers.js:10
 10:   process.stdout.clearLine()
                      ^^^^^^^^^ property `clearLine`. Property not found in
 10:   process.stdout.clearLine()
       ^^^^^^^^^^^^^^ tty$WriteStream

node/lib/readline.js
node/lib/tty.js

I'm not sure if I added them in the correct place, so please check and let me know.

@nmote nmote added the Library definitions Issues or pull requests about core library definitions label Jan 3, 2019
@nmote
Copy link
Contributor

nmote commented Jan 25, 2019

Could you link API docs to support this change? If it's an undocumented API in Node (even though it does exist in the source) we would have to think twice about merging this.

@nmote nmote self-assigned this Jan 25, 2019
@reyronald
Copy link
Contributor Author

Hey @nmote , sure, should've done that from the get-go, sorry bout that!

I'm certain those are not undocumented APIs, I think this might be what you are looking for

Also, for context: nodejs/node-v0.x-archive#6948

@nmote
Copy link
Contributor

nmote commented Jan 28, 2019

We already have cursorTo and clearLine defined on the readline module (search for declare module "readline" in lib/node.js). It looks like that is what the docs you linked are referring to.

Since there aren't any docs which support this change, I'm going to close for now. Feel free to reopen if I have misunderstood something or if you find different docs.

@nmote nmote closed this Jan 28, 2019
@reyronald
Copy link
Contributor Author

reyronald commented Feb 10, 2019

Hey @nmote ,

I guess then I should've linked to these docs instead, on the tty.WriteStream class (my bad):

There are for sure missing definitions for these two functions, I even found code on this repo itself using them when I was researching the issue after getting Flow errors on my project and before opening this PR:

if (this.isTTY) {
// $FlowFixMe Add to lib
process.stdout.moveCursor(0, -this.fancyStatusSize);
// $FlowFixMe Add to lib
process.stdout.cursorTo(0);
// $FlowFixMe Add to lib
process.stdout.clearLine();

if (process.stdout.isTTY) {
// $FlowFixMe - Add this to lib file
process.stdout.clearLine();

Also note that:

The tty.WriteStream class is a subclass of net.Socket that represents the writable side of a TTY. In normal circumstances, process.stdout and process.stderr will be the only tty.WriteStream instances created for a Node.js process and there should be no reason to create additional instances.
Source

I believe this is the pull request that merged the documentation for these functions on the Node repo: nodejs/node#22893

So I think this is still missing from Flow and should be addressed, but I can't re-open this issue apparently. Let me know what you think!

@goodmind goodmind reopened this Jul 28, 2019
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@mroch has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@mroch merged this pull request in 4b30713.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Library definitions Issues or pull requests about core library definitions Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants