repl: make REPLServer.bufferedCommand private #13687

lance · 2017-06-14T20:04:09Z

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

refack · 2017-06-14T20:10:36Z

lib/repl.js

  self.lines.level = [];

+  self.clearBufferedCommand = function clearBufferedCommand() {


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?

refack · 2017-06-14T20:12:12Z

lib/repl.js

@@ -72,6 +72,7 @@ for (var n = 0; n < GLOBAL_OBJECT_PROPERTIES.length; n++) {
  GLOBAL_OBJECT_PROPERTY_MAP[GLOBAL_OBJECT_PROPERTIES[n]] =
    GLOBAL_OBJECT_PROPERTIES[n];
 }
+const BUFFERED_COMMAND = Symbol('bufferedCommand');


Nit: in general kBufferedCommandSymbol is prefered to ALL_CAPS.

vsemozhetbyt · 2017-06-14T20:15:07Z

doc/api/repl.md

@@ -375,6 +375,16 @@ The `replServer.displayPrompt` method is primarily intended to be called from
 within the action function for commands registered using the
 `replServer.defineCommand()` method.

+### replServer.clearBufferedCommand()
+<!-- YAML
+added: v8.1.0


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?

refack · 2017-06-14T20:15:24Z

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

cjihrig

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.

cjihrig · 2017-06-14T20:18:51Z

doc/api/repl.md

+-->
+
+The `replServer.clearBufferedComand()` method clears any command that has been
+buffered but not yet executed.This method is primarily intended to be 


Space after the period please.

cjihrig · 2017-06-14T20:21:20Z

lib/repl.js

  self.lines.level = [];

+  self.clearBufferedCommand = function clearBufferedCommand() {


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?

lance · 2017-06-14T20:30:56Z

@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.

cjihrig

LGTM if this isn't being used.

lance · 2017-06-15T14:33:20Z

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

jasnell · 2017-06-15T18:15:20Z

lib/repl.js

+                        'REPLServer.bufferedCommand is deprecated'),
+    set: util.deprecate((val) => self[kBufferedCommandSymbol] = val,
+                        'REPLServer.bufferedCommand is deprecated')
+  });


add enumerable: true here.

jasnell · 2017-06-15T18:16:34Z

lib/repl.js

+    get: util.deprecate(() => self[kBufferedCommandSymbol],
+                        'REPLServer.bufferedCommand is deprecated'),
+    set: util.deprecate((val) => self[kBufferedCommandSymbol] = val,
+                        'REPLServer.bufferedCommand is deprecated')


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.

jasnell

Needs a bit more work. Left a couple comments.

lance · 2017-06-15T21:56:44Z

@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.

jasnell · 2017-06-16T15:19:43Z

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

lance · 2017-06-19T21:30:50Z

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

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

lance · 2017-06-19T22:44:41Z

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.

lance · 2017-06-21T13:04:34Z

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

@jasnell look ok to you?

Issues addressed

jasnell · 2017-06-22T04:03:08Z

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.

jasnell · 2017-06-22T04:03:49Z

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

lance · 2017-06-27T23:35:10Z

Awaiting CTC review @nodejs/ctc

lance · 2017-07-27T13:49:33Z

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

Trott · 2017-07-27T20:40:29Z

@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.)

Trott · 2017-07-27T20:43:14Z

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

evanlucas · 2017-07-28T00:55:28Z

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

lance · 2017-07-28T16:03:31Z

@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.

mhdawson

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

lance · 2017-08-01T19:46:04Z

Landed in f037cbf

addaleax · 2017-08-01T19:48:35Z

@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. :)

addaleax · 2017-08-01T20:45:57Z

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 <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: James Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Refs: #12686

lance · 2017-08-02T02:13:02Z

@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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

repl: make REPLServer.bufferedCommand private #13687

repl: make REPLServer.bufferedCommand private #13687

lance commented Jun 14, 2017

refack Jun 14, 2017

cjihrig Jun 14, 2017

refack Jun 14, 2017

vsemozhetbyt Jun 14, 2017

lance Jun 14, 2017

vsemozhetbyt Jun 14, 2017

refack commented Jun 14, 2017

cjihrig left a comment

cjihrig Jun 14, 2017

cjihrig Jun 14, 2017

lance commented Jun 14, 2017

cjihrig left a comment

lance commented Jun 15, 2017

jasnell Jun 15, 2017

jasnell Jun 15, 2017

jasnell left a comment

lance commented Jun 15, 2017

jasnell commented Jun 16, 2017

lance commented Jun 19, 2017

lance commented Jun 19, 2017

lance commented Jun 21, 2017

jasnell commented Jun 22, 2017

jasnell commented Jun 22, 2017

lance commented Jun 27, 2017

lance commented Jul 27, 2017

Trott commented Jul 27, 2017

Trott commented Jul 27, 2017

evanlucas commented Jul 28, 2017

lance commented Jul 28, 2017

mhdawson left a comment

lance commented Aug 1, 2017

addaleax commented Aug 1, 2017

addaleax commented Aug 1, 2017

lance commented Aug 2, 2017

		self.lines.level = [];

		self.clearBufferedCommand = function clearBufferedCommand() {

repl: make REPLServer.bufferedCommand private #13687

repl: make REPLServer.bufferedCommand private #13687

Conversation

lance commented Jun 14, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Jun 14, 2017

cjihrig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance commented Jun 14, 2017

cjihrig left a comment

Choose a reason for hiding this comment

lance commented Jun 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

lance commented Jun 15, 2017

jasnell commented Jun 16, 2017

lance commented Jun 19, 2017

lance commented Jun 19, 2017

lance commented Jun 21, 2017

jasnell commented Jun 22, 2017

jasnell commented Jun 22, 2017

lance commented Jun 27, 2017

lance commented Jul 27, 2017

Trott commented Jul 27, 2017

Trott commented Jul 27, 2017

evanlucas commented Jul 28, 2017

lance commented Jul 28, 2017

mhdawson left a comment

Choose a reason for hiding this comment

lance commented Aug 1, 2017

addaleax commented Aug 1, 2017

addaleax commented Aug 1, 2017

lance commented Aug 2, 2017