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

chrisdickinson · 2015-04-23T21:51:54Z

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:

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.
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.
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.
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 · 2015-04-23T21:53:12Z

lib/internal/repl.js

+}
+
+// hack for repl require to work properly with node_modules folders
+module.paths = require('module')._nodeModulePaths(module.filename);


This hack moved here from lib/repl.js.

monsanto · 2015-04-24T00:57:49Z

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 · 2015-04-24T05:35:14Z

Does this supersede #596?

chrisdickinson · 2015-04-24T06:08:06Z

@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 · 2015-04-24T06:17:51Z

@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 · 2015-04-24T11:00:54Z

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 · 2015-04-24T17:29:42Z

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 · 2015-04-24T18:00:02Z

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 · 2015-04-25T01:02:22Z

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 · 2015-04-27T03:19:04Z

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 · 2015-04-27T03:24:50Z

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

indutny · 2015-04-27T07:43:29Z

Finally!

indutny · 2015-04-27T07:44:47Z

doc/api/repl.markdown

+   mode, or a hybrid mode ("magic" mode.) Acceptable values are:
+  * `repl.REPL_MODE_DEFAULT` - Run commands in default mode.
+  * `repl.REPL_MODE_STRICT` - Run commands in strict mode.
+  * `repl.REPL_MODE_MAGIC` - Attempt to run commands in default mode. If they fail


80 column limit?

chrisdickinson · 2015-04-27T18:42:31Z

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

chrisdickinson · 2015-04-29T21:17:31Z

cc @indutny

silverwind · 2015-04-30T16:54:53Z

doc/api/repl.markdown

+  * `repl.REPL_MODE_DEFAULT` - Run commands in default mode.
+  * `repl.REPL_MODE_STRICT` - Run commands in strict mode.
+  * `repl.REPL_MODE_MAGIC` - Attempt to run commands in default mode. If they
+  fail to parse, re-try in strict mode.


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

silverwind · 2015-04-30T18:26:28Z

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

chrisdickinson · 2015-04-30T18:37:56Z

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

silverwind · 2015-04-30T18:42:10Z

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 · 2015-04-30T21:22:05Z

lib/internal/repl.js

+      }
+    }
+
+    fs.open(historyPath, 'w', onhandle);


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

Because we need to trim the output to historySize.

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

The last one that entered a line will win.

indutny · 2015-04-30T21:22:49Z

I have a feature request: ^R key bindings!

indutny · 2015-04-30T21:25:39Z

lib/readline.js

@@ -24,7 +24,7 @@ exports.createInterface = function(input, output, completer, terminal) {
 };


-function Interface(input, output, completer, terminal) {
+function Interface(input, output, completer, terminal, historySize) {


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

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

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.

indutny · 2015-04-30T21:27:06Z

Minor comments, otherwise LGTM. If tests are passing.

indutny · 2015-04-30T22:19:10Z

lib/readline.js

@@ -42,7 +42,9 @@ function Interface(input, output, completer, terminal) {
    completer = input.completer;
    terminal = input.terminal;
    input = input.input;
+    historySize = input.historySize;


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

chrisdickinson · 2015-05-01T00:04:12Z

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

this creates a new internal module responsible for providing the repl created via "iojs" or "iojs -i," and adds the following options to the readline and repl subsystems: * "repl mode" - determine whether a repl is strict mode, sloppy mode, or auto-detect mode. * historySize - determine the maximum number of lines a repl will store as history. The built-in repl gains persistent history support when the NODE_REPL_HISTORY_FILE environment variable is set. This functionality is not exposed to userland repl instances. PR-URL: #1513 Reviewed-By: Fedor Indutny <[email protected]>

chrisdickinson · 2015-05-01T02:33:37Z

Merged in 0450ce7.

rlidwka · 2015-05-01T12:59:25Z

What do you think about those possible improvements?:

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

aredridel · 2015-05-01T13:06:40Z

To replace newlines: nuls.

silverwind · 2015-05-02T19:18:01Z

@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 · 2015-05-02T19:25:31Z

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

cc: @indutny

rlidwka · 2015-05-03T17:07:55Z

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 · 2015-05-04T13:06:45Z

Should be fixed by #1607

fundon · 2015-05-04T13:20:21Z

test/parallel/test-repl-options.js

+  writer: writer,
+  replMode: repl.REPL_MODE_MAGIC,
+  historySize: 50
+})


semicolon ;

@othiym23

PR-URL: #1532 Notable Changes: * crypto: significantly reduced memory usage for TLS (Fedor Indutny & Сковорода Никита Андреевич) #1529 * net: socket.connect() now accepts a 'lookup' option for a custom DNS resolution mechanism, defaults to dns.lookup() (Evan Lucas) #1505 * npm: Upgrade npm to 2.9.0. See the v2.8.4 and v2.9.0 release notes for details. Notable items: - Add support for default author field to make npm init -y work without user-input (@othiym23) npm/npm/d8eee6cf9d - Include local modules in npm outdated and npm update (@ArnaudRinquin) npm/npm#7426 - The prefix used before the version number on npm version is now configurable via tag-version-prefix (@kkragenbrink) npm/npm#8014 * os: os.tmpdir() is now cross-platform consistent and will no longer returns a path with a trailling slash on any platform (Christian Tellnes) #747 * process: - process.nextTick() performance has been improved by between 2-42% across the benchmark suite, notable because this is heavily used across core (Brian White) #1548 - New process.geteuid(), process.seteuid(id), process.getegid() and process.setegid(id) methods allow you to get and set effective UID and GID of the process (Evan Lucas) #1536 * repl: - REPL history can be persisted across sessions if the NODE_REPL_HISTORY_FILE environment variable is set to a user accessible file, NODE_REPL_HISTORY_SIZE can set the maximum history size and defaults to 1000 (Chris Dickinson) #1513 - The REPL can be placed in to one of three modes using the NODE_REPL_MODE environment variable: sloppy, strict or magic (default); the new magic mode will automatically run "strict mode only" statements in strict mode (Chris Dickinson) #1513 * smalloc: the 'smalloc' module has been deprecated due to changes coming in V8 4.4 that will render it unusable * util: add Promise, Map and Set inspection support (Christopher Monsanto) #1471 * V8: upgrade to 4.2.77.18, see the ChangeLog for full details. Notable items: - Classes have moved out of staging; the class keyword is now usable in strict mode without flags - Object literal enhancements have moved out of staging; shorthand method and property syntax is now usable ({ method() { }, property }) - Rest parameters (function(...args) {}) are implemented in staging behind the --harmony-rest-parameters flag - Computed property names ({['foo'+'bar']:'bam'}) are implemented in staging behind the --harmony-computed-property-names flag - Unicode escapes ('\u{xxxx}') are implemented in staging behind the --harmony_unicode flag and the --harmony_unicode_regexps flag for use in regular expressions * Windows: - Random process termination on Windows fixed (Fedor Indutny) #1512 / #1563 - The delay-load hook introduced to fix issues with process naming (iojs.exe / node.exe) has been made opt-out for native add-ons. Native add-ons should include 'win_delay_load_hook': 'false' in their binding.gyp to disable this feature if they experience problems . (Bert Belder) #1433 * Governance: - Rod Vagg (@rvagg) was added to the Technical Committee (TC) - Jeremiah Senkpiel (@Fishrock123) was added to the Technical Committee (TC)

chrisdickinson added the repl Issues and PRs related to the REPL subsystem. label Apr 23, 2015

chrisdickinson reviewed Apr 23, 2015
View reviewed changes

yosuke-furukawa mentioned this pull request Apr 24, 2015

Evangelism: Weekly Update for May 1 nodejs/evangelism#65

Closed

chrisdickinson mentioned this pull request Apr 27, 2015

Release proposal: v2.0.0 #1532

Closed

indutny reviewed Apr 27, 2015
View reviewed changes

silverwind reviewed Apr 30, 2015
View reviewed changes

indutny reviewed Apr 30, 2015
View reviewed changes

chrisdickinson closed this May 1, 2015

fundon reviewed May 4, 2015
View reviewed changes

test/parallel/test-repl-options.js

writer: writer,

replMode: repl.REPL_MODE_MAGIC,

historySize: 50

})

Copy link

Contributor

fundon May 4, 2015

Choose a reason for hiding this comment

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

semicolon ;

robertkowalski mentioned this pull request May 15, 2015

Adding support for .node_history and .noderc.js in $HOME and pwd nodejs/node-v0.x-archive#3178

Closed

stringparser mentioned this pull request May 27, 2015

simpler than gulp-runtime stringparser/gulp-repl#1

Closed

eush77 mentioned this pull request May 31, 2015

Make file format compatible with Node's tmpvar/repl.history#6

Open

This was referenced Nov 25, 2020

[Snyk] Security upgrade snyk from 1.404.0 to 1.425.4 turkdevops/node#34

Open

[Snyk] Security upgrade snyk from 1.404.0 to 1.425.4 turkdevops/node#35

Merged

snyk-bot mentioned this pull request Jul 28, 2021

[Snyk] Upgrade commander from 2.11.0 to 8.0.0 MaxMood96/node#4

Open

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

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

Conversation

chrisdickinson commented Apr 23, 2015

history example

modes example

Choose a reason for hiding this comment

monsanto commented Apr 24, 2015

brendanashworth commented Apr 24, 2015

chrisdickinson commented Apr 24, 2015

monsanto commented Apr 24, 2015

silverwind commented Apr 24, 2015

chrisdickinson commented Apr 24, 2015

silverwind commented Apr 24, 2015

rvagg commented Apr 25, 2015

rvagg commented Apr 27, 2015

chrisdickinson commented Apr 27, 2015

indutny commented Apr 27, 2015

Choose a reason for hiding this comment

chrisdickinson commented Apr 27, 2015

chrisdickinson commented Apr 29, 2015

Choose a reason for hiding this comment

silverwind commented Apr 30, 2015

chrisdickinson commented Apr 30, 2015

silverwind commented Apr 30, 2015

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

indutny commented Apr 30, 2015

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

indutny commented Apr 30, 2015

Choose a reason for hiding this comment

chrisdickinson commented May 1, 2015

chrisdickinson commented May 1, 2015

rlidwka commented May 1, 2015

aredridel commented May 1, 2015

silverwind commented May 2, 2015

silverwind commented May 2, 2015

rlidwka commented May 3, 2015

indutny commented May 4, 2015

Choose a reason for hiding this comment