Node: repl: Undocumented public methods on REPLServer.prototype

[lance]

There are a number of methods on REPLServer.prototype that are clearly available in userland but are not documented. In at least a few cases, my impression is that these methods are attached to the prototype primarily in order to facilitate testing. I think this situation is non-ideal.

• Version: v6.2.2
• Platform: Darwin Lances-MacBook-Pro.local 15.5.0 Darwin Kernel Version 15.5.0: Tue Apr 19 18:36:36 PDT 2016; root:xnu-3248.50.21~8/RELEASE_X86_64 x86_64
• Subsystem: repl, doc

I have seen similar discussions as a part of various issues regarding "semi-private" properties that begin with an underscore on various other modules - though typically those conversations are tangential to the real issue.

This issue is similar - publicly accessible but undocumented properties. The pattern is not limited to REPLServer, but since this is where I have spent most of my time in the code base, it's the one I'm most familiar with now, so for now, this conversation is limited to that module. However, I do think this should be discussed at a higher level and some guidance created for dealing with these "leaks".

As a rule, I would say that no methods should be attached to an object's prototype that are undocumented. But it's not clear how to best deal with existing concerns.

Here is a list of the methods found on REPLServer.prototype that are currently undocumented, but are fully accessible in userland.

REPLServer.prototype.close // inherited from readline.Interface
REPLServer.prototype.setPrompt // inherited from readline.Interface
REPLServer.prototype.createContext
REPLServer.prototype.resetContext
REPLServer.prototype.complete
REPLServer.prototype.parseREPLKeyword
REPLServer.prototype.memory
REPLServer.prototype.convertToContext

I can see why close and setPrompt may not be documented. Although, I think ideally some mention of them should be made in the REPL context since REPLServer provides custom implementations of these methods. In my opinion, the others, however, should be refactored in such a way that they are not accessible in userland, or they should be documented.

[cjihrig]

I did something similar to this recently with the readline module. The process was to:

1. Identify userland usage for each method.
2. Identify which ones should be publicly exposed and which should not.
3. Document the public API methods.
4. Move the others to an internal module and begin deprecating.

[lance]

@cjihrig how on earth do you identify overall userland usage for each method? Surely there's a tool I can use. Can you point me in the right direction?

[cjihrig]

We have a very scientific method of pinging @ChALkeR and asking him to look it up.

[MylesBorins]

@ChALkeR would you be willing to write a document on your process?

[lance]

@mscdex the reason I added the meta tag is that I am also curious about process and/or documentation in the future about best practices among new contributors wrt code style. This may be addressed somewhere already. My experience has been primarily code style enforced by linting - which is good, but I think insufficient.

Maybe these concerns just come out in the LGTM reviews for new pull requests, and that's all of the ceremony needed around it. I just wonder if some semantic code guidelines would be useful.

[lance]

@ChALkeR ping - I'll be happy to take on the work if there's a reasonable way to do this. Noodled a few ideas with some coworkers yesterday, but nothing that was simple and straightforward jumped out.

[lance]

@ChALkeR just following up. Any chance you can lend a hand here?

[ChALkeR]

Ah, sorry, I missed this. Will be ready today =).

[ChALkeR]

_Note: everything here is based on the 2016-01-28 dataset. I collected the newer one (at 2016-07-23), but didn't have enough time to upload it before traveling to another city, so it was just left there on my PC in the last moment :-). I will either rebuild a newer dataset directly on the server or will just use the old one for a few more weeks._

First of all, instances of repl.start (actually, ([rR][eE][pP][lL]|require.*repl.*)\.start) calls are here, to understand what we are working with. Modules extracted from that list — here. All modules that require repl (or anything else similarly named) — here. Their union — here.

• setPrompt and close are hard to check, as readline also has those.
  setPrompt mentions are here. Interersection with the modules list above gives us this list. It's definitely non-empty.

Note that REPLServer is documented to inherit from readline.Interface, and those are documented methods of readline.Interface. I'm pretty sure those should be viewed as being documented.

• convertToContext doesn't look used, see also https://github.com/nodejs/node/pull/7829#issuecomment-234762343. The PR to hard deprecate and removal in the future LGTM. Grep here.
• parseREPLKeyword — here. Only nish seems to use in a way that doesn't copy the repl source code. Also looks good to me to hard deprecate and remove it in the future.
• .memory( calls on anything — here. but only a small part of that is on repl. Intersection (quite small) here, and most of those are node repl copies again. Not all, though.

Still, not sure what to do with that method. Is it worth being documented? I.e. does it bring some functionality that we would want to expose and document?

• .createContext( is hard to search — it's a popular name, e.g. used in vm and other places. Searching for \.createContext[^a-zA-Z], removing what definitely looks like calls on vm. ((vm_?|Script)\.createContext) and intersecting with the module list gives us this list, which looks like just node repl copies and false positives. Most probably it would be fine to hard deprecate that, if we are sure that it shouldn't be documented and exposed.
• .resetContext — here. Filtered — here. Most of those look like node repl copies, but ejdb doesn't. Most probably it would be fine to hard deprecate that, if we are sure that it shouldn't be documented and exposed.

[ChALkeR]

@lance @TheAlphaNerd @cjihrig
Sorry for the delay again. Wish GitHub had a different category for personal mentions in the notifications.

[cjihrig]

Revisiting https://github.com/nodejs/node/issues/7619#issuecomment-231389294. Could we document the process of looking up usage? If a specific dataset is needed, perhaps we could use some foundation money to host it somewhere. If @ChALkeR were to get hit by a bus, or even come down with the flu, this whole process of identifying usage currently gets blocked.

[ChALkeR]

@cjihrig I'm trying to host the scripts at https://github.com/ChALkeR/Gzemnid, but it's only slowly transitioning from a set of hacky bash scripts that require manual actions to something more sensible. Perhaps will push some updates soon. That also works as a server to allow searches using a web API. I had it running as web service some time ago at http://gzemnid.oserv.org/, but it's not ready yet, and atm it's down.

Perhaps I will spend a bit more time on that in the coming days.

The 2016-01-28 dataset is hosted at http://oserv.org/npm/Gzemnid/2016-01-28/ — the *.lzo files and the bash script. Anyone could download the pre-built dataset and obtain exactly the same greps.

Also that repo provides the deep dependency checker (to build lists like this), pre-built files are named deps-nested.json in other folders.

Perhaps we need a separate issue for that somewhere.

[cjihrig]

Hm, maybe it's time to create a separate repo under the nodejs org. We're clearly already leaning on this functionality, and moving it under the org might get additional contributors.

[ChALkeR]

@cjihrig I will try to make that one working without any additional non-documented actions, document it a bit (at least list the commands), and then it'll be at least usable by anyone other than myself. I'm all for moving that into the org once it reaches a «usable» state.

[cjihrig]

Thanks @ChALkeR. I've opened #7935 as a separate place for discussion. Sorry for hijacking this issue :-)

[lance]

@ChALkeR no worries on the delay. I agree that GH's mentions need a better UI, and had assumed that you (like everyone else) has to deal with a deluge. Thanks for doing this!

[jasnell]

setPrompt() and close() I would consider to be documented and left as is.

convertToContext() is already in the process of being hard deprecated.

I would likely recommend doing a soft-deprecation of parseREPLKeyword() and memory() in v7 with an eye towards hard-deprecation in v8. This would give current potential users some runway to transition off. This would mean adding documentation for these methods but with an explicit deprecation note included.

createContext() and resetContext() are a bit murkier for me. Perhaps for these we should simply document the methods with an eye towards soft-deprecation post-v7 if it becomes apparent that there are no obvious external use cases for them.

[Trott]

@lance Should this issue remain open?

[lance]

@Trott sorry for the long delay - this slipped through the cracks for me. I will create a PR to address deprecating the few properties that @jasnell mentioned.

[lance]

@jasnell I've submitted a PR for parseREPLKeyword(). I will add documentation for createContext() and resetContext(), but I think maybe https://github.com/nodejs/node/issues/14226 should be fixed first, since I don't want to document buggy behavior.

With regard to memory(), I can't even figure out how I would document this or why a user would want to call it. I am open to suggestion, but I don't think that my explanation of that method would be very accurate or concise.

[lance]

I think this issue can be closed now with the following PRs landed:

• repl: deprecate REPLServer.parseREPLKeyword
• repl: remove REPLServer.createContext side effects
• repl: deprecate REPLServer.prototype.turnOffEditorMode