readline: deprecate undocumented exports

[cjihrig]

This commit moves the readline's undocumented exports into an internal module. The existing public exports are given a deprecation notice.

Refs: #3847
Fixes: #3836

R= @jasnell

[jasnell]

LGTM
Technically this would likely need to be semver-major.

[ChALkeR]

Looks like semver-major to me.

[cjihrig]

Yes, semver-major for sure

[ChALkeR]

Are four functions mentioned in #3836 the only exports removed in this commit?

[cjihrig]

The four functions from #3836, plus emitKeys(), which is already private and needed by emitKeypressEvents().

[ChALkeR]

Hm… I am not sure if those are unused. Will post greps later, probably tomorrow.
Maybe we should expose deprecated versions for a release.

[cjihrig]

There are deprecation messages in place for all of them.

[ChALkeR]

@cjihrig Ah, sorry, I missed that =), was too sleepy yesterday.
Should be good, then, I guess. I will do some greps later today.

[tejasmanohar]

👍

[cjihrig]

CI - https://ci.nodejs.org/job/node-test-pull-request/764/

[jasnell]

CI is red but failures look unrelated. Do we want to go ahead and land or give it a bit?

[ChALkeR]

@jasnell Give me some time, I will take a quick look at the usage of those methods.

[jasnell]

@ChALkeR : +1

[jasnell]

@nodejs/ctc ... a quick sanity check review on this would be appreciated

[mscdex]

I think we should at least keep the style consistent here, preferably with each argument on a separate line (for better readability).

[mscdex]

One minor nit, otherwise LGTM.

[ChALkeR]

https://gist.github.com/ChALkeR/9cd0a398093bc6161d36 — emitKeypressEvents.
https://gist.github.com/ChALkeR/91f1988ed0e7d6065abb — getStringWidth.
https://gist.github.com/ChALkeR/d31f41f06eb3a5193585 — stripVTControlCharacters.
https://gist.github.com/ChALkeR/d5d686522ec36a860c95 — isFullWidthCodePoint.

[ChALkeR]

Out of those, emitKeypressEvents seems used by several modules. Is there a reason why it should?

[jasnell]

@ChALkeR ... I'm sorry, is there a reason why it should what?

[cjihrig]

Would there be value to leaving emitKeypressEvents() public and documenting it? the other functions seem to mostly be helpers, while emitKeypressEvents() may add real value.

[jasnell]

@cjihrig ... that's definitely a good possibility. I'd be ok with that.

[ChALkeR]

@jasnell Sorry, English is not my native language.

I wanted to ask if there are any specific reasons why several modules have to call emitKeypressEvents, which was meant to be an internal method. If those modules have a valid reason for calling it, we could consider adding it to the API instead.

Note: I'm not familiar with repl code, guessing that just looking at the actual usage in modules.

[jasnell]

@ChALkeR ... no worries :-) There may or may not be good reasons for modules calling it but given that they are and we made the mistake of not explicitly marking those as private using the _ prefix, we may as well just go ahead and document that one and keep it as @cjihrig suggests. The other ones I think we're safe deprecating and moving .

[ChALkeR]

@jasnell It's directly used by only 7 modules (plus the false negatives), so deprecating it shouldn't be such a problem. The question is — is that method a valuable addition the the API that we could want to keep?

[cjihrig]

The ability to emit keypress events seems useful to me.

[cjihrig]

Undid changes regarding emitKeypressEvents(). New CI: https://ci.nodejs.org/job/node-test-pull-request/791/