Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

repl: add repl.setupHistory for programmatic repl #25895

Closed
wants to merge 2 commits into from

Conversation

lance
Copy link
Member

@lance lance commented Feb 2, 2019

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.

@lance lance added the repl Issues and PRs related to the REPL subsystem. label Feb 2, 2019
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Feb 2, 2019
@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 2, 2019
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Adds a `repl.setupHistory()` instance method so that
programmatic REPLs can also write history to a file.

This change also refactors all of the history file
management to `lib/internal/repl/history.js`, cleaning
up and simplifying `lib/internal/repl.js`.
@lance lance force-pushed the repl-programmatic-history branch 2 times, most recently from 059a3ec to bef8550 Compare February 4, 2019 19:11
doc/api/repl.md Outdated Show resolved Hide resolved
@danbev
Copy link
Contributor

danbev commented Feb 8, 2019

@lance lance self-assigned this Feb 8, 2019
@lance
Copy link
Member Author

lance commented Feb 11, 2019

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

lance added a commit that referenced this pull request Feb 11, 2019
Adds a `repl.setupHistory()` instance method so that
programmatic REPLs can also write history to a file.

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

PR-URL: #25895
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@lance
Copy link
Member Author

lance commented Feb 11, 2019

Landed in: 0aa7444

@lance lance closed this Feb 11, 2019
addaleax pushed a commit that referenced this pull request Feb 13, 2019
Adds a `repl.setupHistory()` instance method so that
programmatic REPLs can also write history to a file.

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

PR-URL: #25895
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@targos targos mentioned this pull request Feb 14, 2019
@jasonkarns
Copy link
Member

jasonkarns commented Mar 2, 2019

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
Copy link
Member Author

lance commented Mar 2, 2019

@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
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

lance commented Mar 4, 2019

@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
Copy link
Member

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
Copy link
Member Author

lance commented Mar 4, 2019

@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
Copy link
Member

jasonkarns commented Mar 4, 2019

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
Copy link
Member Author

lance commented Mar 4, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. repl Issues and PRs related to the REPL subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants