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.

Sign up for GitHub

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

Use file descriptor 0 to read stdin #198

Closed
wants to merge 1 commit into from
Closed

Use file descriptor 0 to read stdin #198

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 22 additions & 5 deletions bin/commonmark
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import util from "util";
import fs from "fs";
import os from "os";
import readline from "readline";
import * as commonmark from "../lib/index.js";
var version = require('../package.json').version;
import parseArgs from "minimist";
Expand Down Expand Up @@ -77,11 +78,27 @@ if (format === 'html') {
}

if (files.length === 0) {
if (os.platform() === "win32") {
inps.push(fs.readFileSync(0, 'utf-8'));
} else {
inps.push(fs.readFileSync('/dev/tty', 'utf-8'));
}
if (process.stdin.isTTY) {
if (os.platform() === 'win32') {
// Windows does not have a PTY that Node relies on.
// Instead, we read lines explicitly, tracking ^Z for EOF.
(async () => {
const rl = readline.createInterface({ input: process.stdin });
let inp = '';
for await (const line of rl) {
if (line === '\x1a') { break; }
inp += line;
}
const doc = parser.parse(inp);
const rendered = renderer.render(doc);
if (!options.time) { process.stdout.write(rendered); }
Comment on lines +92 to +94
Copy link
Member

Choose a reason for hiding this comment

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

Does this belong here? Aren't we going to fall through to line 109 and expect inps to contain an array of strings?
I am clueless about how async stuff works in JS, but is there a way to do this synchronously to make the code more straightforward?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will fall through, but with a files array that is empty (a requirement of the top if condition), so the rest of the code prints nothing.

There is no built-in way to do things synchronously with readline.
Node.js encourages asynchronous operation as it was its main philosophical tenet.
The fs API is an exemption to the rule.

There are external libraries that I think can do that, such as readline-sync.
It seems to do some gnarly background-process spawning.

We can also convert the whole file to asynchronous operation, which is a more significant patch.

Copy link
Member

Choose a reason for hiding this comment

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

I see. It would be nice to avoid duplicating the parsing and rendering code like this -- if it takes making the whole thing async, maybe that's the best approach.

Copy link
Member

Choose a reason for hiding this comment

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

Well, maybe it's not worth the effort. I can just merge this.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I think this is not good as it is. You fall through and then end up parsing another empty document and printing the result of rendering it. I'd want to avoid that.

})();
} else {
inps.push(fs.readFileSync('/dev/tty', 'utf-8'));
}
} else {
inps.push(fs.readFileSync(0, 'utf-8'));
}
} else {
for (i = 0; i < files.length; i++) {
var file = files[i];
Expand Down