repl: add repl.setupHistory for programmatic repl

[lance]

Adds a repl.setupHistory() instance method so that programmatic REPLs can also write history to a file without the need to implement this functionality in userland.

This change also refactors all of the history file management to lib/internal/repl/history.js, cleaning up and simplifying lib/internal/repl.js.

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

This has been done and rejected in the past in #5789. The PR was rejected due to a desire to keep the surface area of the REPL API small. However, there have been additional requests to have this issue reopened here, and I thought I'd give it another go.

If there is consensus on moving forward with this, I will add documentation.

If, ultimately, we decide not to merge this, I think the refactoring of the history file functionality into lib/internal/repl/history.js is worthwhile, makes the code easier to read, and does not change the API.

EDIT documentation has been added to repl.md. I'm not sure what to put for the version number in the documentation YAML. I've made it v11.9.1, but not sure if this just gets modified when landing. I couldn't find anything in the collaborator's guide which addresses this issue.

[nodejs-github-bot]

@lance build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2492/pipeline

[addaleax]

Apart from not having documentation yet, this LGTM and I'm 👍 on having the feature

[danbev]

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

[lance]

Final CI after latest commits: https://ci.nodejs.org/job/node-test-pull-request/20728/

[lance]

Landed in: 0aa7444

[jasonkarns]

It looks like this feature was added while not adhering to the XDG basedir spec? What steps are necessary to get XDG adopted as a requirement for future features? It's rather frustrating as a user to see progress made (npm's config location configurable via env var, node-gyp) only to backslide whenever node writes a new file somewhere.

#2224 (comment)
#2224 (comment)
#11907 (comment)

The problematic line: 0aa7444#diff-8b3e69eaa9660fb13cfc197d34eafbfbR33

[lance]

@jasonkarns that line wasn't added, just moved from here. It was added four years ago in this commit. I think if you want the .node_history file to conform to this spec, it would need to be a separate pull request and would likely be a semver-minor or semver-major change.

[jasonkarns]

Prior to this PR, setupHistory was only invoked once, and it respected NODE_REPL_HISTORY.

Now setupHistory is a public api function that doesn't respect NODE_REPL_HISTORY so any invocations of it can't be configured (by the user) to use a particular file location. So now it's possible for utilities that leverage repl to store history at path.join(os.homedir(), '.node_repl_history'); without respecting a user's configuration.

[jasonkarns]

A potential (untested) fix would be to have this line respect NODE_REPL_HISTORY first and then fall back to path.join(os.homedir(), '.node_repl_history')

[lance]

@jasonkarns I am not really following here, sorry. When invoking REPL.start(), setupHistory() is still only invoked once and does respect NODE_REPL_HISTORY. This occurs here. So, while it is true that the setupHistory() method itself does not take into account NODE_REPL_HISTORY, its invocation during default REPL creation does. Behavior in the default case has not changed.

The only thing this pull request did to change behavior is to allow users of a REPL created programatically, to set a history file of their choosing. I see your concern about not recognizing NODE_REPL_HISTORY explicitly in the context of this method, but that does not prevent a user from calling it like this.

repl.setupHistory(env.NODE_REPL_HISTORY, cb);

[jasonkarns]

The user of the repl API will be cli module authors. The users who have files written to their home directory will be users of those modules.

Sure, nothing stops the module authors from being good citizens. But I think the repl API would be more friendly to the final end users if being a good citizen were opt-OUT instead opt-IN. If the repl API is going to provide a default value at all (which it presently does), then I strongly think that default value should respect end user configuration. That way, module authors are "good citizens" by default, instead of "bad citizens" by default.

[lance]

@jasonkarns bear with me for a moment please. I really don't understand what problem you are trying to solve.

If I add a check here for process.env.NODE_REPL_HISTORY as you suggest, then that means the cli module author called setupHistory() with either null or undefined as a parameter. It is true that in that case the history file will default to process.env.NODE_REPL_HISTORY as you suggest, but that seems a pretty roundabout way to achieve this result.

As a cli module author, it would seem odd to me that if I wanted output to be written to the process.env.NODE_REPL_HISTORY file location, I should pass null or undefined to repl.setupHistory(). That's pretty counter-intuitive, and the outcome would be no different than if I called repl.setupHistory(process.env.NODE_REPL_HISTORY, cb).

If null or undefined is not passed as a parameter, but instead it's a string, then the author is explicitly setting the output to a file location of their choice and process.env.NODE_REPL_HISTORY should be ignored anyway.

[jasonkarns]

If null or undefined is not passed as a parameter, but instead it's a string, then the author is explicitly setting the output to a file location of their choice and process.env.NODE_REPL_HISTORY should be ignored anyway.

Agree.

As a cli module author, it would seem odd to me that if I wanted output to be written to the process.env.NODE_REPL_HISTORY file location, I should pass null or undefined to repl.setupHistory()

Agree that it's not obvious. But as a module author, if I were reading the code at face value without observing the history, it would seem to me that it's an intentional behavior (albeit undocumented). Otherwise, why would there be a default fallback for falsy, non-string at all?

I'm sure you've encountered numerous other weird APIs in your history where passing in false, null, or 0 is an intentional (albeit, non-intuitive) supported use case. I know I have. (I can remember one offhand in node's own codebase where 0 was an intentional stand-in for Infinity)

So while it may not be documented, or even likely, Hyrum's Law applies:

With a sufficient number of users of an API, it does not matter what you promise in the contract, all observable behaviors of your system will be depended on by somebody.

So I'm suggesting that, for as long as it's even possible for the repl.setupHistory to provide a default value, that default should respect the end user's configured environment.

Or alternatively, I guess, eliminate the default behavior entirely from the public method [1]. But I would much prefer the former; since that could pave the way for a potential future documented way for module authors to fallback to defaults (in an end-user-friendly way).

[1] AFAICT, the falsy fallback really only exists to satisfy env.NODE_REPL_HISTORY being unset. (Indeed, that's already a bit buggy, IMO, because it treats empty string and unset differently; despite most common practice in shell of using FOO= as "unset") The public API's defaulting behavior could be removed entirely by moving it here with something roughly like: env.NODE_REPL_HISTORY || path.join(os.homedir(), '.node_repl_history')

(Sorry, as the discussion grows, I probably should have opened a separate issue that could facilitate a corresponding PR. Should I still?)

Also, I apologize for my initial tone as it bore out some general frustration with so many utilities and whatnot continuing to ignore/be-unaware-of/violate the XDG spec. Thanks for the new repl history feature in the first place. 🙇 ❤️

[lance]

(Sorry, as the discussion grows, I probably should have opened a separate issue that could facilitate a corresponding PR. Should I still?)

Yes, I think this would be best. I would definitely like more eyes on it, since I am generally -0 on your suggested change. I have a local change with a test that fits the bill, but I do not want to push it until a wider discussion occurs.

Also, I apologize for my initial tone as it bore out some general frustration with so many utilities and whatnot continuing to ignore/be-unaware-of/violate the XDG spec. Thanks for the new repl history feature in the first place. bowing_man heart

No worries. :)