REPL – Add "magic" mode detection, persistent history support

[chrisdickinson]

This PR adds two public API features: the ability to configure the maximum history size of readline, and the ability to change the "mode" a REPL is started in – where "mode" may be "strict mode", "sloppy mode", or "magic / auto-detect mode". Auto-detect mode catches SyntaxErrors due to V8's lack of support for certain constructs (classes, let, and const) outside of strict mode, and retries the utterance in strict mode. User REPLs default to "sloppy mode" as before.

It also adds an internal module that controls the built-in CLI REPL. This module augments the REPL instance with the following features:

1. If NODE_REPL_HISTORY is set to a file path, it will attempt to use that file to back up history in JSON format. This punts on the painful issue of trying to determine a user's home directory in a cross-platform way.
2. If NODE_REPL_HISTORY is not set, pressing "up" (or "ctrl p") in the CLI REPL when there are no other lines in history will print instructions on how to set up persistent history support. It will only print this message once per session. The goal is to capture intent and communicate a way to enable support.
3. If NODE_REPL_MODE is set, it will start the REPL in that mode. The options are strict, sloppy, and magic. If not given, the REPL will start in "magic" mode. The goal is to reduce surprise when folks get the new V8 and try to write a class in the REPL immediately.
4. If NODE_REPL_HISTORY_SIZE is set and is a Number, it will set the maximum history to that size.

None of these features interact in any way with the REPL module, and are strictly additive for the CLI only.

Putting this up for an early review to make sure the features (and approach) are acceptable.

R=@rvagg

history example

modes example

First, "sloppy" mode (default as of old iojs), then "strict" mode, then "magic" mode.

[chrisdickinson]

This hack moved here from lib/repl.js.

[monsanto]

Wait, why are we punting on cross-platform home directories? This isn't a hard task, and it doesn't matter if we do it perfectly. On Unix $HOME or bust is fine.

Providing a nice out-of-the-box experience for a new user is more important than avoiding a bit of "hacky" (not even) code. Let us please learn from the Python community on this one, they did not have tab completion or persistent history on by default for the longest time, and the only good it did was for the owners of StackOverflow.

[brendanashworth]

Does this supersede #596?

[chrisdickinson]

@monsanto IIRC, previous PRs introducing persistent history have stalled for a few reasons: first among them is that they introduce persistent history to the repl or readline modules itself, thus affecting a much larger swath of downstream code. Not far behind that is that most PRs rely on being able to resolve the user's home directory in a cross-platform way, which appears to be fairly involved – or at least, is beyond the scope of adding history support. This PR attempts to avoid those obstacles by:

• Making the CLI repl a special-case, internal module – these changes should not affect repls users have embedded in their applications.
• Punting on the home directory issue by making the history feature opt-in via an environment variable. A path for discovery is provided (via the "press up with no history" warning) making the feature discoverable but not invasive. If, in the future, we introduce a mechanism for finding the home directory of the user, we can easily transition to automatically enabling history at ~/.node_history.

So, in other words, yes, I agree that we should eventually target the user's home directory automatically. This PR sets us up to do that once the other pieces of the puzzle fall into place, and lets us prove out the feature before exposing it to everyone automatically.

@brendanashworth ah, possibly. Sorry, I didn't see that one linked from the original topic.

[monsanto]

@chrisdickinson If home directory detection is such a contentious issue I can bring it up in a different issue. Having an environment variable is a good idea anyway so I suppose having a sensible default is an orthogonal issue.

[silverwind]

I'm on @monsanto's side here. Persistent history should 'just work'.

Home directory detection isn't hard, and we don't have to make it public API unless we feel it's flawless (this and performance were the main concerns in #682).

Another issue with introducing environment vars is you have to keep them around for at least a semver-major cycle if we want to deprecate them later in favor of automatic detection.

As for REPL mode, I'd like to see strict or auto as the default mode. At least I once expected let to work without tinkering, but that's more of a v8 fault too. Who writes non-strict code nowadays anyways?

[chrisdickinson]

Persistent history has been hamstrung by those issues before. This is a temporary compromise that lets us improve functionality for end users. Even when we add home detection it should be paired with an env var to override the location, so we shouldn't have to worry about deprecating those vars.

re: modes, it defaults the CLI shell to "magic/auto" mode to avoid affecting existing repl usage. strict mode throws errors on undeclared vars which is not desirable in a repl.

On Apr 24, 2015, at 4:00 AM, silverwind [email protected] wrote:

I'm on @monsanto side here. Persistent history should 'just work'.

Home directory detection isn't hard, and we don't have to make it public API unless we feel it's flawless (this and performance were the main concerns in #682).

Another issue with introducing environment vars is you have to keep them around for at least a semver-major cycle if we want to deprecate them later in favor of automatic detection.

As for REPL mode, I'd like to see strict or auto as the default mode. At least I expected let to work without tinkering, but that's more of a v8 fault than ours :)

—
Reply to this email directly or view it on GitHub.

[silverwind]

Sounds good, the override through env will be useful and I'd really like to see persistency finally landed.

Regarding home detection based on env: Would those that opposed it last time be okay with it being private to the CLI spawned shell?

Also, +1 on auto mode default.

[rvagg]

I guess this seems fine to me, avoiding the home directory stuff for now seems like a pragmatic way to make progress here, we can improve over time (an approach we need to take more - including the use of feature flags - whereby we do part of a solution/feature with the expectation it'll improve over time rather than trying to do the perfect job in one leap)

[rvagg]

I'll give a lgtm for "magic mode" but abstain from the history changes for now, perhaps others feel more strongly either way re history

[chrisdickinson]

cc @indutny / @bnoordhuis – could I get an additional review of this?

[indutny]

Finally!

[indutny]

80 column limit?

[chrisdickinson]

Thanks for the review! Updated to address comments – PTAL.

[chrisdickinson]

cc @indutny

[silverwind]

Maybe put a note that strict mode is the same as 'use strict'? Also should probably not capitalize 'Run' and 'Attempt' here.

[silverwind]

Docs so far look okay. I assume doc for the env variables will follow?

[chrisdickinson]

@silverwind Yeah – though I'm a little unsure as to where those would get documented.

[silverwind]

Probably a seperate issue, but I think having all env variables in one seperate markdown file would be nice. As for this issue, I'd probably put them somewhere after the first paragraph.

[indutny]

Could you please explain why we can't just have a+ fd? Because of multiple repl instances?

[chrisdickinson]

Because we need to trim the output to historySize.

[indutny]

Gotcha. A question: if multiple repls will be opened simultaneously, which of them will "win"?

[chrisdickinson]

The last one that entered a line will win.

[indutny]

I have a feature request: ^R key bindings!

[indutny]

Gosh, this is so frustrating... Why do we need one more argument? Can't it be an options-only thing?

[chrisdickinson]

We should add that to the v3.0.0 milestone, I think.

[indutny]

Well, you are adding it here. Why not take the historySize from the options object, and otherwise use default value? I don't like adding one more argument to this function.

[chrisdickinson]

Will do.

[indutny]

Minor comments, otherwise LGTM. If tests are passing.

[indutny]

I mean let's leave this here, but remove the argument.

[chrisdickinson]

All comments addressed. Will merge this evening. Thanks all!

[chrisdickinson]

Merged in 0450ce7.

[rlidwka]

What do you think about those possible improvements?:

1. We could use plaintext format like ~/.bash_history, so people can more easily grep from it. Not sure what to replace newline with though.
2. Use appendFile as discussed here. This way separate repl instances would be able to work together (hopefully).
3. Trim history file only on the first load, not every time it writes there.

[aredridel]

To replace newlines: nuls.

[silverwind]

@chrisdickinson just tried the persistent history feature, but couldn't get it to work. Two issues:

• No matter what I tried, the history file wasn't written to. I created it manually but that didn't help either. The variable was correctly identfied by process.env.NODE_REPL_HISTORY_PATH. Not sure what else to tell you.
• The docs refer to NODE_REPL_HISTORY_FILE but the code uses NODE_REPL_HISTORY_PATH.

[silverwind]

Looks like opts.terminal is undefined for me at the point this variable is checked. This is a regular iterm2 terminal.

cc: @indutny

[rlidwka]

I think this commit also broke debugger:

alex@y:~/io.js$ ./iojs debug ./test.js _debugger.js:760 this.repl = repl.start(opts); ^ TypeError: repl.start is not a function at new Interface (_debugger.js:760:20) at Object.exports.start (_debugger.js:28:20) at startup (node.js:64:9) at node.js:881:3 

[indutny]

Should be fixed by #1607

[fundon]

semicolon ;