StdIO + runCommand (with CaptureIO)

[facekapow]

I believe I got it working WITHOUT conflicts. I resolved a couple (if not all) of the issues commented on in #60, and took into account the possible improvements. @kesla I removed the callback from runCommand, it now just returns the events so you can load it into a variable and use it from there. @iefserge I made a StdIO class, with a CaptureIO module for runCommand, with runtime.stdio being an instance of StdIO. I also pushed a commit that creates a stdio global and makes runtime.stdio point to it.

EDIT: I just pushed a commit that adds stdnet, and splits stdio up. Now there's 4 new globals:

• stdout
• stdin
• stdnet
• and stdio ...instead of just stdio.

I think this is better since you can have multiple functions with the same name in different ios (ex: write in both stdout and stdnet).

[facekapow]

Wait, gotta fix it.

[kesla]

What's the reasoning behind stdnet? Also, @iefserge do we really want runtime-core to use node.js-http? To me it feels like runtime should be lower level than that

[facekapow]

@kesla Well, the idea was that since runtime.js doesn't have any filesystem (@iefserge said he was going to remove VFS), it should be supported by default to download files, and later store them into buffers. stdnet is going to do that, although, now that you mention it, maybe it should be an optional module as well. Yeah, I'll put it into a module.

[kesla]

@ArielAbreu If you use runtimeify you get a working require('http') so I then don't understand what the value is to wrap it into core. So I guess it depends on how you define "by default". I think that you should be able to download files using runtimeify, but not by only using runtimejs.

[facekapow]

@kesla Yes, but if I create a module, I can add it to runtimeify's builtins and do what you mean, using runtimeify to allow downloading files.

EDIT: I hate autocorrect on Mac, the commit message below is supposed to say "Remove stdnet, fix stdio", NOT "Remove student, fix stdio".

[facekapow]

@kesla @iefserge I created runtime-utils, a module containing utils for runtimejs. Right now, it's only util is stdnet.

[iefserge]

I think runtime-utils should not be necessary, we should be able to use https://www.npmjs.com/package/request for file downloads.

[facekapow]

Well, yes. I'll remove the stdnet from runtime-utils. But, it listens for res.on('end'), which doesn't work because there is no destroy() function in runtime-node-net. I created a PR in runtime-node-net that fixes this which I need @kesla to approve.

[iefserge]

@kesla yes, I think node compatibility should be provided as an extra module(s).

[facekapow]

@iefserge I moved the globals into an env global, but CaptureIO is still there because it's easier to manage and fix in a separate file, plus it needs to be created with new so that each command has it's own special properties. I can make it an object, but i'm pretty sure it would break stdin.

[facekapow]

@iefserge I've completely reimplemented the IO based on your comments. A little example:

'use strict';

var runtime = require('runtimejs');

runtime.shell.setCommand('foobar', function(args, env, cb) {
  // get input
  env.stdin.readLine(function(text) {
    // echo the input
    env.stdout.writeLine(text);
    // Send return code 0:
    cb(0);
  });
});

var myout = runtime.stdio.createOut({
  onwrite: function(text) {
    // it's obvious what this does:
    // "hey! it's output: sometext"
    env.stdout.writeLine("hey! it's output:", text);
  }
});

var myin = runtime.stdio.createIn({
  onreadline: function(cb) {
    // send 'blah blah text' to the callback as the input
    cb('blah blah text');
  }
});

// Run 'foobar' with 'myout' as stdout and 'myin' as stdin
runtime.shell.runCommand('foobar', {
  stdout: myout,
  stdin: myin
}, function(code, redrawPrompt) {
  // code can be 0, and...
  // 0 == false, so check for null specifically
  if (code !== null) {
    env.stdout.writeLine('exited with code ' + code);
    // redraw the prompt
    redrawPrompt();
  }
});

This code will output: "hey! it's output: blah blah text" "exited with code 0" (without the quotes, of course)

[iefserge]

Looks very nice! :+1: I'm going to do a more detailed review soon.

[facekapow]

@iefserge Maybe the input box listener could be configured to use runCommand() so that it can get the exit code of the app. For anything non-zero (undefined would count as zero), a red X could be displayed at the beginning of the prompt (like some zsh themes do).

[facekapow]

@iefserge I've added a feature (?) that displays a red X at the beginning of the prompt when the exit code is non-zero (and, therefore, runtime.shell now uses runCommand to run commands and get the exit code). I also fixed a bug that allows you to delete the prompt after an app uses read or readline, the fix is to create new instances of shared/input.js every time it's used. Also fixed another bug where the text caret would stay when using readline (it's still present in read, though).

[facekapow]

@iefserge I added a commit that catches errors in runCommand and passes them to the callback Node.js style. So the runCommand part of the example above is now:

// ...
runtime.shell.runCommand('foobar', {
  stdout: myout,
  stdin: myin
}, function(err, code, redrawPrompt) {
  if (err) {
    // handle the error
  }
  // code can be 0, and...
  // 0 == false, so check for null specifically
  if (code !== null) {
    env.stdout.writeLine('exited with code ' + code);
    // redraw the prompt
    redrawPrompt();
  }
});
// ...

[iefserge]

@ArielAbreu hey, sorry for the delay. I left some somments, overall it looks very nice

[facekapow]

@iefserge I think I fixed everything (correct me if i'm wrong). I also removed the try and catch, but there needs to be a way to stop the function on error and prevent errors from being thrown, otherwise, the VM has to be restarted.

[facekapow]

Guess I forgot quite a few things 😁

[facekapow]

@iefserge I think I fixed the problems, plus I also fixed the shell commands.

[iefserge]

@ArielAbreu tried to merge, I get

Uncaught exception: /bundle.js:473: TypeError: Cannot read property 'Stdout' of undefined
TypeError: Cannot read property 'Stdout' of undefined
    at Object.7 (/bundle.js:473:37)
    at s (/bundle.js:1:254)
    at /bundle.js:1:305
    at Object.<anonymous> (/bundle.js:425:14)
    at Object.6../initio (/bundle.js:455:4)
    at s (/bundle.js:1:254)
    at /bundle.js:1:305
    at Object.<anonymous> (/bundle.js:5373:15)
    at Object.59../core (/bundle.js:5405:4)
    at s (/bundle.js:1:254)

npm run lint outputs some minor code-style issues.

[iefserge]

I can take care of this if you'd like.

[facekapow]

Sorry, just got back. I'm gonna take a look.

[facekapow]

@iefserge I fixed the undefined issue, had to move the code in initio outside the exported function into the exported function. I was being run before runtime was defined, that's why it was giving an error. I also fixed the new runtime.tty.Stdout() part, since Stdout() is now in runtime.stdio.

[facekapow]

@iefserge I also tried to fix some issues with lint, but I got a merge conflict, so I decided to undo the commit (since it's only minor, plus the code works without it).

[piranna]

Just a question/sugestion: should be the stdio, stdout & stderr concepts moved out of the tty, and maybe also from runtime.js core concepts? In fact, the tty only understand about input stream (keyboard) and output stream (screen), so the diferences between stdout and stderr are only software abstractions and could be at the same level of the colors management...

My two cents.

[facekapow]

Maybe. Note: right now, there is no stderr (maybe I should add it, not sure).

[piranna]

Ok :+1:

[iefserge]

@piranna I think there needs to be some interface in the core that could be used by plugins (pluggable shells, REPLs, they could possibly interact with each other). It could also be theoretically used for other kinds of input/output in the core. stderr could be easily provided by another Stdout object, not sure if it's necessary though.

[piranna]

stderr could be easily provided by another Stdout object, not sure if it's necessary though.

Python implement stdio as a File object that when read return the data from the keyboard and when written put in in the screen (similar to a Node.js Duplex stream object), and all stdin, stdout and stderr are references to the same File object, so stdout and stderr are the same.

I think the core should be as minimalist as possible, mostly a wrapper over the hardware. From an API perspective, there could be only one write stream and stdout could accept only string and buffer objects while stderr could capture the Error objects written to it, this way you only have a stream with several kinds of data instead of several streams. I think it's easier to manage...

[iefserge]

@piranna @ArielAbreu hm, I like the idea to merge stdout and stdin into one Stdio object to simplify things. write/writeLine functions convert all input to strings. We could add writeError specifically for errors and let the receiver figure out what to do with them (tty could print error.stack using red color, for example).

[facekapow]

What do you mean "convert all input to strings". Isn't write output all they do? What do they have to do with input? (but, again, maybe, first I need a little idea of what you mean).

[iefserge]

@ArielAbreu this is how it works right now, I think it's nice that write converts any arguments to strings.

[facekapow]

I thought you meant user input, my bad.

[iefserge]

@ArielAbreu I'd say let's merge Stdout and Stdin into one Stdio object (rename and expose class as runtime.stdio.StdioInterface and default stdio object that interacts with tty as runtime.stdio.defaultStdio). Other than that, this looks good!

[facekapow]

I'll combine them.

[facekapow]

@iefserge I've combined Stdout and Stdin into one Stdio class available as runtime.stdio.Interface, with a default io as runtime.stdio.defaultio (with runtime.tty.io as an alias). It also has a new writeError() method that accepts either a string or an Error object and prints it's stack property.

I've also been thinking there should be a way of sending data between applications. Maybe a global event object could be exposed that allows events to sent to applications like:

runtime.events.send(appname, 'someevent', mydatavar);

// someapp
runtime.shell.setCommand('appname', function(args, env, cb) {
  env.on('someevent', function(datavar) {
    // i don't know, do something with `datavar`
  });
});

I don't know, maybe i'm getting ahead of myself. 😐 Just a random idea.

[iefserge]

@ArielAbreu Very nice! However I have a few comments related to API design.

1.The issue with calling it Interface is that you cannot simply do

obj instanceof Interface

to check the object type and I think it's very confusing that class name is different from the property name visible to user. For example, there is runtime.net.TCPSocket, class named TCPSocket and obj instanceof TCPSocket works correctly. I think we should follow the same rule across the API.

2.Let's remove the alias, I would argue that this creates unnecessary confusion. I'd say we should have every object exposed no more than once.

3.defaultio consists of two words, it should probably be named either defaultIO or maybe just defaultStdio, because defaultIO could refer to different kinds of io, not just "standard IO". Simply default would also work (is this a good idea to use a js keyword though?). I think we should either call it io or stdio everywhere consistently (env property as well). Please, let's call it stdio because io is very vague :)

I'm sorry, I should've documented the rules currently used by other APIs somewhere so we can make sure it's consistent. We can change these rules of course, but we would have to update all other APIs as well.

writeError is nice! :+1: Let's think about events and communication separately from this feature, this could get very complicated.

[facekapow]

@iefserge I've fixed the issues. Sorry about the API, I guess everyone has their own way of doing things.

[piranna]

I guess everyone has their own way of doing things

Absolutely :-D Me, I would have splitted the screen and the keyboard from the stdio and would have implemented it as an object combining them :-)

[iefserge]

@ArielAbreu Great work!! Merged!

[iefserge]

@piranna that's pretty much what StdioInterface is for, it's in no way connected to a screen and keyboard. defaultStdio is the instance that implements this interface using screen/kbd :)

[piranna]

@piranna that's pretty much what StdioInterface is for, it's in no way connected to a screen and keyboard. defaultStdio is the instance that implements this interface using screen/kbd :)

Ah cool! :-D