repl: Proposal for better repl (implementation) by princejwesley · Pull Request #9601 · nodejs/node

princejwesley · 2016-11-14T05:24:08Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

repl

Description of change

Welcome message with version and help guide
- displayWelcomeMessage flag is used to
  turn on/off
Differentiate execute & continue actions
- ^M or enter key to execute the command
- ^J to continue building multiline expression.
- executeOnTimeout value is used to determine
  the end of expression when terminal is false.
Pretty stack trace.
- REPL specific stack frames are removed before
  emitting to output stream.
Recoverable errors.
- No more recoverable errors & no false positives.
Defined commands(like .exit, .load)
- Meaningful only at the top level.
- .break is removed - no more get stuck

Welcome message template

$ node
Welcome to Node.js <<version>> (<<vm name>> VM, <<vm version>>)
Type ^M or enter to execute, ^J to continue, ^C to exit
Or try .help for help, more at https://nodejs.org/dist/<<version>>/docs/api/repl.html

>

Pretty stack trace

$ node -i
> throw new Error('tiny stack')
Error: tiny stack
    at repl:1:7
> var x y;
var x y;
      ^
SyntaxError: Unexpected identifier
>

Refs: #8195
Original WIP PR: #8504

jasnell · 2016-11-18T15:36:41Z

@princejwesley +1 on this... it's going to take some time to review properly tho. Will hopefully have some comments soon

fhinkel · 2017-03-26T12:20:24Z

@princejwesley Would it make sense to rebase this?

princejwesley · 2017-03-26T14:45:12Z

@fhinkel sure, I'll do

* Welcome message with version and help guide * `displayWelcomeMessage` flag is used to turn on/off * Differentiate execute & continue actions * ^M or enter key to execute the command * ^J to continue building multiline expression. * `executeOnTimeout` value is used to determine the end of expression when `terminal` is false. * Pretty stack trace. * REPL specific stack frames are removed before emitting to output stream. * Recoverable errors. * No more recoverable errors & no false positives. * Defined commands(like .exit, .load) are meaningful only at the top level. * Remove `.break` command. Welcome message template ------------------------ ```js $ node Welcome to Node.js <<version>> (<<vm name>> VM, <<vm version>>) Type ^M or enter to execute, ^J to continue, ^C to exit Or try ``` Pretty stack trace ------------------ ```js $ node -i > throw new Error('tiny stack') Error: tiny stack at repl:1:7 > var x y; var x y; ^ SyntaxError: Unexpected identifier > ```

princejwesley · 2017-04-02T20:00:27Z

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

TimothyGu · 2017-04-02T20:38:11Z

lib/repl.js

 exports.REPL_MODE_SLOPPY = Symbol('repl-sloppy');
 exports.REPL_MODE_STRICT = Symbol('repl-strict');
-exports.REPL_MODE_MAGIC = exports.REPL_MODE_SLOPPY;
+exports.REPL_MODE_MAGIC = Symbol('repl-strict');


Was this change intended?

TimothyGu · 2017-04-02T20:39:11Z

lib/repl.js

+    const docURL = `https://nodejs.org/dist/${version}/docs/api/repl.html`;
+
+    const welcome = `Welcome to Node.js ${version} (${jsEngine} VM,` +
+                    ` ${jsEngineVersion})\nType ^M or enter to execute,` +


I think Node.js ${version} (${jsEngine} ${jsEngineVersion}) should suffice; the "VM" seems a bit extraneous.

TimothyGu · 2017-04-02T20:40:08Z

lib/repl.js

-    replMap.set(magic, replMap.get(this));
    magic.context = magic.createContext();
-    flat.run(tmp);                        // eval the flattened code
+    flat.run(tmp, magic);                        // eval the flattened code


The comment indentation is now out of date.

TimothyGu · 2017-04-02T20:41:33Z

lib/repl.js


+function indexOfInternalFrame(frames) {
+  return frames.indexOf('vm.js') !== -1 ||
+    frames.indexOf('REPLServer.') !== -1;


Can this interfere with userland script files named vm.js?

TimothyGu · 2017-04-02T20:42:24Z

lib/repl.js

-}
-inherits(Recoverable, SyntaxError);
-exports.Recoverable = Recoverable;
+}, 'replServer.convertToContext() is deprecated');


The DEP0024 code was removed accidentally.

TimothyGu · 2017-04-03T07:07:33Z

doc/api/repl.md

+    displayed. Defaults to `false`.
+```js
+> node
+Welcome to Node.js <<version>> (<<vm name>> VM, <<vm version>>)


The VM should be gone now.

TimothyGu · 2017-04-03T07:07:42Z

doc/api/repl.md

 ```js
 $ node
-> const a = [1, 2, 3];
+Welcome to Node.js v6.5.0 (v8 VM, 5.1.281.81)


TimothyGu · 2017-04-03T07:09:36Z

lib/repl.js

-  this.run = function(data) {
-    for (var n = 0; n < data.length; n++)
-      this.emit('data', `${data[n]}\n`);
+  this.run = (data, repl) => {


Can this function be put into the prototype object?

TimothyGu · 2017-04-03T07:17:08Z

doc/api/repl.md

+* `<ctrl>-C` - When pressed once, it will break the current command.
  When pressed twice on a blank line, has the same effect as the `.exit`
  command.
 * `<ctrl>-D` - Has the same effect as the `.exit` command.


^M and ^J should be documented.

TimothyGu · 2017-04-03T07:20:48Z

doc/api/repl.md

 repl.start({prompt: '> ', eval: myEval});
 ```

-#### Recoverable Errors


I for one am not very happy this feature got removed. In recent versions of Chrome, a similar function just got added (instead of having to type Shift+Enter, the prompt automatically recognizes line continuation), and to have to type an awkward Ctrl+J for the same feature does not sound very appealing to me.

If this change is here to stay, may I suggest we at least add support to Shift+Enter also/instead? It is the syntax in most IM apps, and also in most developer consoles.

@TimothyGu we discussed this option here. Shift+Enter is not possible

Comments addressed

BridgeAR · 2017-08-26T09:38:28Z

Ping @princejwesley this needs a rebase. Do you still want to follow up on this?

princejwesley · 2017-08-28T01:58:44Z

@BridgeAR @TimothyGu point is valid, it will force user to use extra control key to execute or continue.

lance · 2017-09-06T22:03:20Z

@princejwesley @BridgeAR @TimothyGu

This PR is comprised of a lot of incremental improvements. Some of these are still somewhat controversial. In order to move forward with some of the items that are universally agreed on (e.g. better stack traces and improved welcome message), would it perhaps be better and more fruitful in the short term to submit those as individual PRs? @princejwesley if you are not opposed, I could take a look at this.

TimothyGu · 2017-09-06T23:49:35Z

@lance Agreed.

princejwesley · 2017-09-07T09:31:45Z

@lance sure

lance · 2017-09-14T20:39:56Z

Due to the various reasons noted above, I'm closing this PR. Feel free to reopen if you would like to continue work on it.

When a user executes code in the REPLServer which generates an exception, there is no need to display the REPLServer internal stack frames. PR-URL: #15351 Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Refs: #9601

When a user executes code in the REPLServer which generates an exception, there is no need to display the REPLServer internal stack frames. PR-URL: nodejs/node#15351 Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Refs: nodejs/node#9601

When a user executes code in the REPLServer which generates an exception, there is no need to display the REPLServer internal stack frames. PR-URL: #15351 Backport-PR-URL: #16457 Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Refs: #9601

nodejs-github-bot added debugger readline Issues and PRs related to the built-in readline module. repl Issues and PRs related to the REPL subsystem. labels Nov 14, 2016

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Nov 18, 2016

TimothyGu mentioned this pull request Mar 1, 2017

repl: docs-only deprecate magic mode #11599

Closed

4 tasks

jasnell added the stalled Issues and PRs that are stalled. label Mar 24, 2017

princejwesley force-pushed the repl.enhanced branch from 429e046 to 48414f0 Compare April 2, 2017 19:54

princejwesley removed the stalled Issues and PRs that are stalled. label Apr 2, 2017

TimothyGu previously requested changes Apr 2, 2017

View reviewed changes

princejwesley added 4 commits April 3, 2017 02:28

Accidental change in Magic mode

b34c110

Add back the missing DEP0024

a60da69

Use ContextifyScript instead of vm.js

d6cee38

Remove VM string in welcome message

f304f8c

TimothyGu reviewed Apr 3, 2017

View reviewed changes

Add doc for ^M and ^J

2d5feff

Move ArrayStream.run to prototype

a3047dc

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

Trott mentioned this pull request Jul 11, 2017

repl: Proposal for better repl #8195

Closed

lance mentioned this pull request Sep 12, 2017

repl: remove internal frames from runtime errors #15351

Closed

3 tasks

lance closed this Sep 14, 2017

lance mentioned this pull request Oct 24, 2017

[v8.x backport] repl: remove internal frames from runtime errors #16457

Closed

3 tasks

Uh oh!

Conversation

princejwesley commented Nov 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Welcome message template

Pretty stack trace

Uh oh!

jasnell commented Nov 18, 2016

Uh oh!

fhinkel commented Mar 26, 2017

Uh oh!

princejwesley commented Mar 26, 2017

Uh oh!

princejwesley commented Apr 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Aug 26, 2017

Uh oh!

princejwesley commented Aug 28, 2017

Uh oh!

lance commented Sep 6, 2017

Uh oh!

TimothyGu commented Sep 6, 2017

Uh oh!

princejwesley commented Sep 7, 2017

Uh oh!

lance commented Sep 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

princejwesley commented Nov 14, 2016 •

edited

Loading