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

doc: readline.emitKeypressEvents note #9447

Closed
wants to merge 1 commit into from

Conversation

STRML
Copy link
Contributor

@STRML STRML commented Nov 3, 2016

Affected core subsystem(s)

doc

Description of change

It is not noted in the docs that emitKeypressEvents is automatically called on any input stream where input.terminal or input.isTTY is true.

This can cause incompatibility with libraries that fire their own keypress events, like blessed.

I am working to fix the incompatibility but I thought it was worth noting in the docs.

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. readline Issues and PRs related to the built-in readline module. labels Nov 3, 2016
@@ -447,6 +447,10 @@ autocompletion is disabled when copy-pasted input is detected.

If the `stream` is a [TTY][], then it must be in raw mode.

*Note* This is automatically called by any readline instance on its `input`,
Copy link
Member

Choose a reason for hiding this comment

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

A few small nitpicks:

Nit: Put a : after *Note* to match the other five instances in the doc: *Note*: This is automatically...

Nit: Remove the comma after input.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was modeling that after the Note on L30 - I'll fix that too.

@@ -447,6 +447,10 @@ autocompletion is disabled when copy-pasted input is detected.

If the `stream` is a [TTY][], then it must be in raw mode.

*Note* This is automatically called by any readline instance on its `input`,
if the `input` is a terminal. Cleaning up the readline does not stop the
Copy link
Member

Choose a reason for hiding this comment

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

Is there wording that might be more precise than "cleaning up"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep - will fix

@STRML
Copy link
Contributor Author

STRML commented Nov 3, 2016

Amended

@@ -447,6 +447,10 @@ autocompletion is disabled when copy-pasted input is detected.

If the `stream` is a [TTY][], then it must be in raw mode.

*Note*: This is automatically called by any readline instance on its `input`
if the `input` is a terminal. Closing the readline does not stop the `input`
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this?:

Closing the readline instance does not...

...instead of:

Closing the readline does not...

@Trott
Copy link
Member

Trott commented Nov 3, 2016

Formatting etc. all LGTM. One tiny style nit. Maybe someone else can review that the content is correct, applies across all platforms, etc. (Judging from the git logs, perhaps one of @piscisaureus @rlidwka @isaacs @princejwesley?)

@STRML
Copy link
Contributor Author

STRML commented Nov 3, 2016

Thanks for the review. Updated again.

@@ -447,6 +447,10 @@ autocompletion is disabled when copy-pasted input is detected.

If the `stream` is a [TTY][], then it must be in raw mode.

*Note*: This is automatically called by any readline instance on its `input`
Copy link
Contributor

Choose a reason for hiding this comment

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

Note after code snippet ?

@@ -447,6 +447,10 @@ autocompletion is disabled when copy-pasted input is detected.

If the `stream` is a [TTY][], then it must be in raw mode.

*Note*: This is automatically called by any readline instance on its `input`
if the `input` is a terminal. Closing the `readline` instance does not stop
Copy link
Contributor

Choose a reason for hiding this comment

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

@Trott Sounds like a bug to me.

  function onNewListener(event) {
    if (event == 'keypress') {
      stream.on('data', onData);
     // +++++++++ MISSING +++++++++++
      iface.once('close', () => {
        stream.removeListener('data', onData);
      });
     // +++++++++++++++++++++++++++++
      stream.removeListener('newListener', onNewListener);
    }
  }

Copy link
Member

Choose a reason for hiding this comment

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

Ping @Trott @princejwesley ... any further thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure why I I'm being pinged on this (as opposed to anybody else) to be honest...

Copy link
Member

Choose a reason for hiding this comment

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

(Also not sure who a good person to ping about it would be, though. ¯\(ツ)/¯ )

Copy link
Member

Choose a reason for hiding this comment

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

(Also not sure who a good person to ping about it would be, though. ¯\(ツ)/¯ )

I think with hindsight we can say @addaleax would be a good person to ping 😉

Copy link
Member

Choose a reason for hiding this comment

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

Well … what I remember about these parts of the code mostly comes from reviewing ae17883, and that was a year ago. I guess at this point we don’t have any real readline experts…

@jasnell
Copy link
Member

jasnell commented Mar 24, 2017

@nodejs/ctc ... anyone have thoughts on this?

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Mar 24, 2017
@fhinkel
Copy link
Member

fhinkel commented May 26, 2017

There hasn't been any activity here. I'm closing this. Feel free to reopen (or ping a collaborator) if I closed this in error.

@fhinkel fhinkel closed this May 26, 2017
@STRML
Copy link
Contributor Author

STRML commented May 27, 2017 via email

@fhinkel fhinkel reopened this May 27, 2017
gibfahn added a commit to gibfahn/node that referenced this pull request May 28, 2017
Once the Readline interface is closed, the 'data' event listener should
be removed.

Refs: nodejs#9447 (comment)
@gibfahn
Copy link
Member

gibfahn commented May 28, 2017

Note: This is automatically called by any readline instance on its input if the input is a terminal.

This seems to be correct looking at the code, so that part LGTM

Closing the readline instance does not stop the input from emitting 'keypress' events.

I'd rather fix this than document it (assuming it's a bug), see #13266

jasnell pushed a commit that referenced this pull request Jun 1, 2017
Once the Readline interface is closed, the 'data' event listener should
be removed.

PR-URL: #13266
Ref: #9447 (comment)
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Jun 5, 2017
Once the Readline interface is closed, the 'data' event listener should
be removed.

PR-URL: #13266
Ref: #9447 (comment)
Reviewed-By: James M Snell <jasnell@gmail.com>
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

This is not a bug, see #13560 (for a bit of explanation and a test that shows why we can’t remove the decoder once it’s there).

Thanks for catching this and sorry that this has been open so long! I’ll wait a bit to see if there’s any movement in the other issues (edit: before landing, I mean).

@addaleax addaleax removed the stalled Issues and PRs that are stalled. label Jun 8, 2017
@addaleax
Copy link
Member

addaleax commented Jun 10, 2017

Landed in ac34779, thank you for the PR!

@addaleax addaleax closed this Jun 10, 2017
addaleax pushed a commit that referenced this pull request Jun 10, 2017
PR-URL: #9447
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
addaleax pushed a commit that referenced this pull request Jun 12, 2017
PR-URL: #9447
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@addaleax addaleax mentioned this pull request Jun 12, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@MylesBorins
Copy link
Contributor

is this applicable to v6.x?

MylesBorins pushed a commit that referenced this pull request Sep 19, 2017
PR-URL: #9447
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Sep 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. readline Issues and PRs related to the built-in readline module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants