Prompt fix #22
Prompt fix #22
Conversation
| const parse = parse_; | ||
|
|
||
| import { Terminal } from "xterm"; | ||
|
|
There was a problem hiding this comment.
All this imports were not used, so I removed them
| this.wasmerFileSystem.volume.fds[0].read = this.stdinRead.bind(this); | ||
| this.wasmerFileSystem.volume.fds[1].write = this.stdoutWrite.bind(this); | ||
| this.wasmerFileSystem.volume.fds[2].write = this.stdoutWrite.bind(this); | ||
|
|
||
| if (sharedStdin) { | ||
| this.sharedStdin = sharedStdin; |
There was a problem hiding this comment.
This was not necessary since the params are already typed with the same types as the object types
|
Note: this PR breaks the commands |
|
Edit: |
There was a problem hiding this comment.
Left some comments! Would appreciate if you would read through them @syrusakbary 😄
Merging as I will be the one to address the comments 😂
| commandHistory: Array<string>; | ||
| commandHistoryIndex: number; | ||
|
|
||
| localEcho: LocalEchoController; |
There was a problem hiding this comment.
Can we rename the local echo service? When I hear "echo" I think of the command echo, which doesn't really make sense to me that it handles input for xterm? 🤔 😄
| const parse = parse_; | ||
|
|
||
| import { Terminal } from "xterm"; | ||
|
|
| @@ -132,8 +124,11 @@ export default class CommandRunner { | |||
| (window as any).SharedArrayBuffer && (window as any).Atomics; | |||
|
|
|||
| this.xterm = xterm; | |||
| this.localEcho = new LocalEchoController(this.xterm, { | |||
There was a problem hiding this comment.
So the suffix Controller I think is a little too verbose. Also, in the webdev world, Controllers tend to refer to angularJS Controllers, which is usually associated with a DOM Component. I think this is more of a Service if we were to add a suffix. 🤔 😄 👍
| processStdinReadCallback() {} | ||
| processStdinReadCallback() { | ||
| // When the stdin read begins | ||
| // console.log( |
There was a problem hiding this comment.
I tend to remove debug code when submitting PRs. In general, I tend to agree with leaving old commented code is bad. I feel it is just noise, and it is the reason why git history exists 😄
Also, especially in terms of console logs, a log that makes sense to you, will not for me because i don't have context on where the log is coming from 👍
| /** | ||
| * The history controller provides an ring-buffer | ||
| */ | ||
| export class HistoryController { |
There was a problem hiding this comment.
Probably would like to rename this to CommandHistoryService. 😄
| @@ -44,18 +44,14 @@ export default class WASICommand extends Command { | |||
|
|
|||
| this.wasmerFileSystem = new WasmerFileSystem(); | |||
|
|
|||
| // Bind our stdinRead | |||
| // Bind our stdinRead / stdoutWrite | |||
| } else { | ||
| this.stdinReadCounter = 0; | ||
| } | ||
| if (this.isReadingStdin) { |
There was a problem hiding this comment.
Thank you for solving the mystery 🔍 😂
| }, | ||
| "dependencies": { | ||
| "@types/shell-quote": "^1.6.1", | ||
| "local-echo": "github:wavesoft/local-echo", |
There was a problem hiding this comment.
Oh nice, didn't know this existed, glad someone made it: https://github.com/wavesoft/local-echo 😄 🔥 🔥 🔥
Still think the naming should be different. Perhaps, XtermInputHandler.
Controller does make some sense in this case, but i'd still be wary since it has nothing to do with DOM directly.
| @@ -93,5 +93,10 @@ | |||
| "typescript": "^3.5.3", | |||
| "url": "^0.11.0", | |||
| "xterm": "^3.14.5" | |||
| }, | |||
| "dependencies": { | |||
| "@types/shell-quote": "^1.6.1", | |||
There was a problem hiding this comment.
So Right now you are adding dependencies to the lib, that are actually needed to build the shell.
See npm dependencies.
If someone were to try and install the Wasi lib (Which the package.json represents), they would then also need to install these dependencies. When really, these are not needed by the lib, but are required by the shell. Some of these dependencies would be "kind of" correct once we split out the terminal into its own lib as well with lerna. But as of now, I don't think this is. 😄 👍
| "dependencies": { | ||
| "@types/shell-quote": "^1.6.1", | ||
| "local-echo": "github:wavesoft/local-echo", | ||
| "shell-quote": "^1.7.1" |
There was a problem hiding this comment.
This is rad too: https://www.npmjs.com/package/shell-quote
😄 👏 🎉
|
Merging! 😄 |
|
Also, just another note to myself / @syrusakbary , FireFox and Safari get stuck in an infinite |
This PR refactors the stdin prompt reads into it's own utility module (
local-echo) - inspired by https://www.linkedin.com/pulse/xtermjs-local-echo-ioannis-charalampidis (repo: https://github.com/wavesoft/local-echo).It also fixed QuickJS prompts so they work seamlessly.
