repl: make REPLServer.bufferedCommand private by lance · Pull Request #13687 · nodejs/node · GitHub

The REPLServer.bufferedCommand property was undocumented, except
for its usage appearing, unexplained, in an example for
REPLServer.defineCommand. This commit deprecates that property,
privatizes it, and adds a REPLServer.clearBufferedCommand()
function that will clear the buffer.

Refs: #12686

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

repl, doc

IMHO it would be better to add clearBufferedCommand to REPLServer.prototype.

About Object.defineProperty(this, 'bufferedCommand' I'm not sure since that will change the .hasOwnProperty('bufferedCommand')

Does this function need to be exposed at all? Would it work to create a function in this file that has the symbol in scope and takes the REPL as an argument?

Nit: in general kBufferedCommandSymbol is prefered to ALL_CAPS.

Should this probably be REPLACEME till landing? Anyway, even 8.1.1 has not this fix.

Yes. I wasn't sure what to put here (and didn't realize 8.1.1 was released yesterday).

I am not sure about rules here. Maybe it is usually replaced on backporting from master to current?

Added some comments.
As for the PR in general I'm +1, but we'll need a wide consensus.

Do you foresee this being something that is needed at all outside of core? Also, I'm not sure that we can just remove bufferedCommand with no notice.

Space after the period please.

Does this function need to be exposed at all? Would it work to create a function in this file that has the symbol in scope and takes the REPL as an argument?

@cjihrig I really only see the bufferedCommand being useful outside of core in cases where users have used REPLServer.defineCommand and want to clear what may exist within the buffer. This is the doc example in which it was exposed, and it seems more break-y to remove it completely vs. a deprecation.

LGTM if this isn't being used.

Love to get another LGTM on this - @jasnell @addaleax?

add enumerable: true here.

This would need an assigned deprecation code. See doc/api/deprecations.md.
Before this lands, make the code DEP00XX and whom ever does the landing would assign the actual code.
The code would be passed in as the third argument for the util.deprecate() method.

Needs a bit more work. Left a couple comments.

@jasnell thanks - I've added a temporary deprecation code to util.deprecate but it wasn't clear from your comment if I should also add this to deprecations.md or if that happens when the PR lands.

Yes, an entry should be added to deprecations.md using the temporary code.

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

@jasnell documentation bits added.
@Fishrock123 any opinion on this?

Hmnm feebsd test failed with

not ok 535 parallel/test-http-agent-keepalive
  ---
  duration_ms: 0.753
  severity: crashed
  stack: |-
    oh no!
    exit code: CRASHED (Signal: 11)

which seems unrelated.

'nother CI https://ci.nodejs.org/job/node-test-commit/10701/

@jasnell look ok to you?

Issues addressed

I've cleared my review. I'd like to get more input from @nodejs/ctc about whether this is something we definitely want to do.

ping @ChALkeR ... any way to get a usage analysis

Awaiting CTC review @nodejs/ctc

@Trott any movement on this from @nodjs/ctc?

@lance Can you rebase to get rid of conflicts? (I'm guessing it's all white-space stuff based on new stricter indentation linting. Be sure to run make jslint before pushing to GitHub to resolve the conflicts.)

Needs one more CTC approval. Pinging some CTC folks who have several commits modifying lib/repl.js: @indutny @bnoordhuis @mscdex @addaleax @thefourtheye @evanlucas

https://github.com/search?l=JavaScript&p=2&q=bufferedCommand&type=Code&utf8=%E2%9C%93 has some real world use cases for this currently. Are we sure that we can justify this breaking change? I'm on the fence

Relates to #12686 The `REPLServer.bufferedCommand` property was undocumented, except for its usage appearing, unexplained, in an example for `REPLServer.defineCommand`. This commit deprecates that property, privatizes it, and adds a `REPLServer.clearBufferedCommand()` function that will clear the buffer.

Change to kBufferedCommandSymbol instead of BUFFERED_COMMAND. Made `clearBufferedCommand` exist on `REPLServer.prototype`. Minor typo corrections.

and make REPLServer.bufferedCommand enumerable

@evanlucas there are a lot of results returned from that query, but the vast majority of them are for jquery-buffer.js which is unrelated. I skimmed through the first 10 pages of results, and other than jquery-buffer.js, most of the references fall into two categories.

Setting replServer.bufferedCommand to ''.
Clones or other copies of the Node.js codebase - not forks, copies.

The first case is covered by the addition of REPLServer.clearBufferedCommand(), and the second will not be affected by this change.

I tried to figure out a way to eliminate jquery-buffer.js from search results, but my GitHub search foo is not quite there.

Given the usage data and that its only deprecation at this point it seems reasonable to me.

Landed in f037cbf

@lance I un-landed this PR because the DEP00XX should have been replaced while landing… if you want to re-land with that, please do so, otherwise I can. :)

Re-landed in 2ca9f94

I realized I made the same mistake a couple of days ago, maybe that was a bit confusing … sorry!

The `REPLServer.bufferedCommand` property was undocumented, except for its usage appearing, unexplained, in an example for `REPLServer.defineCommand`. This commit deprecates that property, privatizes it, and adds a `REPLServer.clearBufferedCommand()` function that will clear the buffer. PR-URL: #13687 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <mhdawson@ibm.com> Reviewed-By: Ruben Bridgewater <ruben.bridgewater@fintura.de> Refs: #12686

@addaleax I realized my mistake after I walked away from my computer today. Thanks for fixing it up.

lance added doc Issues and PRs related to the documentations. repl Issues and PRs related to the REPL subsystem. labels Jun 14, 2017

refack added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 14, 2017

refack reviewed Jun 14, 2017

View reviewed changes

vsemozhetbyt reviewed Jun 14, 2017

View reviewed changes

cjihrig requested changes Jun 14, 2017

View reviewed changes

cjihrig approved these changes Jun 14, 2017

View reviewed changes

danbev approved these changes Jun 15, 2017

View reviewed changes

jasnell reviewed Jun 15, 2017

View reviewed changes

jasnell previously requested changes Jun 15, 2017

View reviewed changes

BridgeAR approved these changes Jun 21, 2017

View reviewed changes

jasnell added the ctc-review label Jun 22, 2017

Trott mentioned this pull request Jun 25, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-06-28 nodejs/CTC#148

Closed

Trott mentioned this pull request Jul 2, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-07-05 nodejs/CTC#153

Closed

Trott mentioned this pull request Jul 16, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-07-19 nodejs/CTC#154

Closed

lance added 4 commits July 28, 2017 11:46

repl: addresses PR comments

85ca3f6

Change to kBufferedCommandSymbol instead of BUFFERED_COMMAND. Made `clearBufferedCommand` exist on `REPLServer.prototype`. Minor typo corrections.

add REPLServer.bufferedCommand deprecation code

55a7a64

and make REPLServer.bufferedCommand enumerable

doc: REPLServer.bufferedCommand deprecation

714bcd4

Trott mentioned this pull request Jul 31, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-08-02 nodejs/CTC#160

Closed

jasnell approved these changes Jul 31, 2017

View reviewed changes

mhdawson approved these changes Aug 1, 2017

View reviewed changes

lance closed this Aug 1, 2017

lance deleted the 12686-hide-buffered-command-repl branch August 1, 2017 19:46

Trott removed the ctc-review label Aug 6, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

repl: make REPLServer.bufferedCommand private by lance · Pull Request #13687 · nodejs/node · GitHub

Uh oh!

Uh oh!

repl: make REPLServer.bufferedCommand private - #13687

repl: make REPLServer.bufferedCommand private#13687lance wants to merge 4 commits intonodejs:masternodejs/node:masterfrom lance:12686-hide-buffered-command-replCopy head branch name to clipboard

Conversation

lance commented Jun 14, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Jun 14, 2017

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lance commented Jun 14, 2017

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

lance commented Jun 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

lance commented Jun 15, 2017

Uh oh!

jasnell commented Jun 16, 2017

Uh oh!

lance commented Jun 19, 2017

Uh oh!

lance commented Jun 19, 2017

Uh oh!

lance commented Jun 21, 2017

Uh oh!

jasnell commented Jun 22, 2017

Uh oh!

jasnell commented Jun 22, 2017

Uh oh!

lance commented Jun 27, 2017

Uh oh!

lance commented Jul 27, 2017

Uh oh!

Trott commented Jul 27, 2017

Uh oh!

Trott commented Jul 27, 2017

Uh oh!

evanlucas commented Jul 28, 2017

Uh oh!

lance commented Jul 28, 2017

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

lance commented Aug 1, 2017

Uh oh!

addaleax commented Aug 1, 2017

Uh oh!

addaleax commented Aug 1, 2017

Uh oh!

repl: make REPLServer.bufferedCommand private#13687
lance wants to merge 4 commits into
nodejs:masterfrom
lance:12686-hide-buffered-command-repl