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 "magic" mode detection, persistent history support #1513

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

chrisdickinson
Copy link
Contributor

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

just-a-test

modes example

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

modes

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

// hack for repl require to work properly with node_modules folders
module.paths = require('module')._nodeModulePaths(module.filename);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This hack moved here from lib/repl.js.

@monsanto
Copy link
Contributor

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

Does this supersede #596?

@chrisdickinson
Copy link
Contributor Author

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

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

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

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 notifications@github.com 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
Copy link
Contributor

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

rvagg commented Apr 25, 2015

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

rvagg commented Apr 27, 2015

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

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

@indutny
Copy link
Member

indutny commented Apr 27, 2015

Finally!

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

Choose a reason for hiding this comment

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

80 column limit?

@chrisdickinson
Copy link
Contributor Author

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

@chrisdickinson
Copy link
Contributor Author

cc @indutny

* `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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@silverwind
Copy link
Contributor

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

@chrisdickinson
Copy link
Contributor Author

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

@silverwind
Copy link
Contributor

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.

}
}

fs.open(historyPath, 'w', onhandle);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we need to trim the output to historySize.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The last one that entered a line will win.

@indutny
Copy link
Member

indutny commented Apr 30, 2015

I have a feature request: ^R key bindings!

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


function Interface(input, output, completer, terminal) {
function Interface(input, output, completer, terminal, historySize) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

@indutny
Copy link
Member

indutny commented Apr 30, 2015

Minor comments, otherwise LGTM. If tests are passing.

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

Choose a reason for hiding this comment

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

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

@chrisdickinson
Copy link
Contributor Author

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 <fedor@indutny.com>
chrisdickinson added a commit that referenced this pull request May 1, 2015
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 <fedor@indutny.com>
@chrisdickinson
Copy link
Contributor Author

Merged in 0450ce7.

@rlidwka
Copy link
Contributor

rlidwka commented May 1, 2015

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

To replace newlines: nuls.

@silverwind
Copy link
Contributor

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

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

cc: @indutny

@rlidwka
Copy link
Contributor

rlidwka commented May 3, 2015

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

indutny commented May 4, 2015

Should be fixed by #1607

writer: writer,
replMode: repl.REPL_MODE_MAGIC,
historySize: 50
})
Copy link
Contributor

Choose a reason for hiding this comment

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

semicolon ;

rvagg added a commit that referenced this pull request May 4, 2015
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.