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

Support repl multiline input #1165

Merged
merged 1 commit into from
Nov 6, 2018
Merged

Support repl multiline input #1165

merged 1 commit into from
Nov 6, 2018

Conversation

hayd
Copy link
Contributor

@hayd hayd commented Nov 6, 2018

This works, but I had to comment out the setTimeout test, maybe due to #1161.

Note: It's not interactive over multiple lines (only the last line is editable).

@hayd hayd changed the title WIP Support repl multiline input Support repl multiline input Nov 6, 2018
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM - I suspect we'll have to go through several revisions of this.

@ry ry merged commit e9327be into denoland:master Nov 6, 2018
@kevinkassimo
Copy link
Contributor

kevinkassimo commented Nov 6, 2018

Yeah we definitely need to improve this later. This solution currently has problems like

> var a = "{";

it won't check if { is in quotes and would instead treat as if user is in a block...

Maybe we could for now differentiate Shift + Return and Return and use the previous to enable multiline input?

@ry
Copy link
Member

ry commented Nov 6, 2018

I would just have a look at how node does this...

@hayd hayd deleted the repl-multiline branch November 6, 2018 21:01
@hayd hayd mentioned this pull request Nov 8, 2018
ry added a commit to ry/deno that referenced this pull request Nov 12, 2018
- Update to TypeScript 3.1.6 (denoland#1177)
- Fixes Headers type not available. (denoland#1175)
- Reader/Writer to use Uint8Array not ArrayBufferView (denoland#1171)
- Fixes importing modules starting with 'http'. (denoland#1167)
- build: Use target/ instead of out/ (denoland#1153)
- Support repl multiline input (denoland#1165)
@ry ry mentioned this pull request Nov 12, 2018
ry added a commit that referenced this pull request Nov 12, 2018
- Update to TypeScript 3.1.6 (#1177)
- Fixes Headers type not available. (#1175)
- Reader/Writer to use Uint8Array not ArrayBufferView (#1171)
- Fixes importing modules starting with 'http'. (#1167)
- build: Use target/ instead of out/ (#1153)
- Support repl multiline input (#1165)
@kevinkassimo
Copy link
Contributor

kevinkassimo commented Nov 29, 2018

@hayd Actually your intuition is correct. Node does the following:
https://github.com/nodejs/node/blob/8344f2458111904ad1de4ce08ffd9b4360ce81ed/lib/repl.js#L655-L668

// If error was SyntaxError and not JSON.parse error
      if (e) {
        if (e instanceof Recoverable && !sawCtrlD) {
          // Start buffering data like that:
          // {
          // ...  x: 1
          // ... }
          self[kBufferedCommandSymbol] += cmd + '\n';
          self.displayPrompt();
          return;
        } else {
          self._domain.emit('error', e.err || e);
        }
      }

Also we might want to implement something similar to Node's vm that relies on binding to V8 context to make let/const work.

Should we implement stuff like vm at current stage?

@hayd
Copy link
Contributor Author

hayd commented Nov 29, 2018

I think we can wait on some compiler improvements (to compile ts) by @kitsonk @bartlomieju , once that is in, it could be refactored to include vm... I'm not sure it makes sense to define until then.

#1169

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants