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

Jump to bottom

readline: add support for FileHandle #43102

Closed
wants to merge 2 commits into from
Closed

readline: add support for FileHandle #43102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9d9c364
readline: add support for `FileHandle`
aduh95 May 14, 2022
63e2120
fixup! readline: add support for `FileHandle`
aduh95 May 14, 2022
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
84 changes: 84 additions & 0 deletions doc/api/readline.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -808,6 +808,36 @@ async function completer(linePartial) {
}
```

### `readlinePromises.createInterface(input)`

<!-- YAML
added: REPLACEME
-->

* `input` {FileHandle}
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the justification behind choosing FileHandle and not a file path or an fd? Is it because most of userland code uses FileHandles nowadays?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using FileHandle is more or less the only reliable way of checking if a file exists to then proceed to read it in an async context, for this reason I think we should simplify its use when we can. fd are just numbers, I think it could be dangerous to add support for those, file paths are just strings. We could consider URL objects, but I'm not convinced that would make a very interesting API.

Copy link
Contributor

Choose a reason for hiding this comment

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

to read it in an async context

Hmm, would be nice to have a test case that tries to read lines from the file after a part of the stream has already been consumed if that's what you mean by the async context here.

Copy link
Contributor Author

@aduh95 aduh95 May 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

By async context, I mean that you at some point need to know that the file exists, then at another point you need to read it and/or write to it. e.g.

async function* getLinesOrCreateFile(path) {
  let fh
  try {
    fh = await open(path, 'r');
  } catch (err) {
    if (err?.code === 'ENOENT') {
      await writeFile(path, getDefaultData());
      yield* getDefaultData();
      return;
    }
    throw err;
  }
  try {
    yield* readLines(fh);
  } finally {
    await fh.close();
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, so something like a cache. This could still be done by accepting string filepaths, fds and url objects with some modifications like opening the file later on in the last try block and turning the open() call into access().

fd are just numbers, I think it could be dangerous to add support for those, file paths are just strings. We could consider URL objects, but I'm not convinced that would make a very interesting API.

I'm not sure how using these are dangerous though. Fd based APIs are not marked as deprecated or legacy and there is no mention of any sort of danger in the fs docs.

I didn't understand what's wrong with file paths being "just strings".

It seems that there are multiple ways in which users could prefer to use this functionality. I feel that adding support for anything other than just readable streams would serve as an impetus for PRs adding support for other things like fds, string paths and even url objects (you might not find the api interesting but someone else might :P). This would unnecessarily expand the API surface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could still be done by accepting string filepaths, fds and url objects with some modifications like opening the file later on in the last try block and turning the open() call into access().

Well no, the access call to a path could report there's a file that exists, then another program could delete it, which would create an error later when trying to read the file. See discussion in #39960 for more context.

I'm not sure how using these are dangerous though. Fd based APIs are not marked as deprecated or legacy and there is no mention of any sort of danger in the fs docs.

#37874 (comment)

I didn't understand what's wrong with file paths being "just strings".

What I mean is readLines('this is a line\nthis is another line') could be something that someone tries, I think it's more useful if it throws a useful error. Anyway, no strong feeling on my side for this, it's not what I'm looking for anyway.

It seems that there are multiple ways in which users could prefer to use this functionality. I feel that adding support for anything other than just readable streams would serve as an impetus for PRs adding support for other things like fds, string paths and even url objects (you might not find the api interesting but someone else might :P). This would unnecessarily expand the API surface.

That's a great point, that's why I suggest instead we make it a method of the FileHandle class.


Convenient alias to create a `readlinePromises.Interface` and stream over the file.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Convenient alias to create a `readlinePromises.Interface` and stream over the file.
Convenient alias to create a `readlinePromises.Interface` and stream over the
file.

lint fix


```mjs
import { open } from 'node:fs/promises';
import { createInterface as readLines } from 'node:readline/promises';
const file = await open('./some/file/to/read');
for await (const line of readLines(file)) {
console.log(line);
}
```

```cjs
const { open } = require('node:fs/promises');
const { createInterface: readLines } = require('node:readline/promises');
(async () => {
const file = await open('./some/file/to/read');
for await (const line of readLines(file)) {
console.log(line);
}
})();
```

## Callback API

<!-- YAML
Expand Down Expand Up @@ -1069,6 +1099,36 @@ function completer(linePartial, callback) {
}
```

### `readline.createInterface(input)`

<!-- YAML
added: REPLACEME
-->

* `input` {FileHandle}

Convenient alias to create a `readline.Interface` and stream over the file.

```mjs
import { open } from 'node:fs/promises';
import { createInterface as readLines } from 'node:readline';
const file = await open('./some/file/to/read');
for await (const line of readLines(file)) {
console.log(line);
}
```

```cjs
const { open } = require('node:fs/promises');
const { createInterface: readLines } = require('node:readline');
(async () => {
const file = await open('./some/file/to/read');
for await (const line of readLines(file)) {
console.log(line);
}
})();
```

### `readline.cursorTo(stream, x[, y][, callback])`

<!-- YAML
Expand Down Expand Up @@ -1208,6 +1268,30 @@ async function processLineByLine() {
processLineByLine();
```

You can also pass directly a `FileHandle` object:

```mjs
import fs from 'node:fs/promises';
import { createInterface as readLines } from 'node:readline';

const myFile = await fs.open('input.txt');
for await (const line of readLines(myFile)) {
console.log(`Line from file: ${line}`);
}
```

```cjs
const fs = require('node:fs/promises');
const { createInterface: readLines } = require('node:readline');

(async () => {
const myFile = await fs.open('input.txt');
for await (const line of readLines(myFile)) {
console.log(`Line from file: ${line}`);
}
})();
```

Alternatively, one could use the [`'line'`][] event:

```js
Expand Down
6 changes: 5 additions & 1 deletion lib/internal/readline/interface.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ const {
} = require('internal/readline/callbacks');

const { StringDecoder } = require('string_decoder');
const { FileHandle } = require('internal/fs/promises');

// Lazy load Readable for startup performance.
let Readable;
Expand Down Expand Up @@ -150,7 +151,10 @@ function InterfaceConstructor(input, output, completer, terminal) {
let prompt = '> ';
let signal;

if (input?.input) {
if (input instanceof FileHandle) {
input = input.createReadStream();
crlfDelay = Infinity;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure that this is the only option that needs to be set with a value different from the default while reading lines from a file?
Also, why isn't Infinity the default value of crlfDelay in all cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are we sure that this is the only option that needs to be set with a value different from the default while reading lines from a file?

That's the documented one, we can always update it later if we find out some are missing.

Also, why isn't Infinity the default value of crlfDelay in all cases?

I don't have the answer to that question, I don't think adding an unnecessary breaking change is worth it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think adding an unnecessary breaking change is worth it

I thought doing that would fix the weird problems you were mentioning in #42590 (comment) instead of introducing unnecessary breakage. (would be interesting to know what the weird problem is though)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The unnecessary breakage is to change the default. See the docs for more context:

node/doc/api/readline.md

Lines 998 to 1003 in 6afd3fc

* `crlfDelay` {number} If the delay between `\r` and `\n` exceeds
`crlfDelay` milliseconds, both `\r` and `\n` will be treated as separate
end-of-line input. `crlfDelay` will be coerced to a number no less than
`100`. It can be set to `Infinity`, in which case `\r` followed by `\n`
will always be considered a single newline (which may be reasonable for
[reading files][] with `\r\n` line delimiter). **Default:** `100`.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is \r\n supposed to be considered as a single line even on posix? I thought line endings were \r on posix and \r\n on windows.

Copy link
Contributor Author

@aduh95 aduh95 May 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

\r and \n are two different characters, \n is the one that marks the end of the line on non-Windows systems. readline will treat both as a line separator, unless there are next to one another.

node/lib/internal/readline/interface.js

Lines 1261 to 1275 in d36b60e

case 'return': // Carriage return, i.e. \r
this[kSawReturnAt] = DateNow();
this[kLine]();
break;
case 'enter':
// When key interval > crlfDelay
if (
this[kSawReturnAt] === 0 ||
DateNow() - this[kSawReturnAt] > this.crlfDelay
) {
this[kLine]();
}
this[kSawReturnAt] = 0;
break;

} else if (input?.input) {
// An options object was given
output = input.output;
completer = input.completer;
Expand Down
41 changes: 41 additions & 0 deletions test/parallel/test-readline-filehandle.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import '../common/index.mjs';
import tmpdir from '../common/tmpdir.js';

import assert from 'node:assert';
import { createInterface as readLines } from 'node:readline';
import { open, writeFile } from 'node:fs/promises';
import path from 'node:path';

tmpdir.refresh();

const filePath = path.join(tmpdir.path, 'file.txt');

await writeFile(filePath, '1\n\n2\n');

let file;
try {
file = await open(filePath);

let i = 0;
for await (const line of readLines(file)) {
switch (i++) {
case 0:
assert.strictEqual(line, '1');
break;

case 1:
assert.strictEqual(line, '');
break;

case 2:
assert.strictEqual(line, '2');
break;

default:
assert.fail();
break;
}
}
} finally {
await file?.close();
}
41 changes: 41 additions & 0 deletions test/parallel/test-readline-promises-filehandle.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import '../common/index.mjs';
import tmpdir from '../common/tmpdir.js';

import assert from 'node:assert';
import { createInterface as readLines } from 'node:readline/promises';
import { open, writeFile } from 'node:fs/promises';
import path from 'node:path';

tmpdir.refresh();

const filePath = path.join(tmpdir.path, 'file.txt');

await writeFile(filePath, '1\n\n2\n');

let file;
try {
file = await open(filePath);

let i = 0;
for await (const line of readLines(file)) {
switch (i++) {
case 0:
assert.strictEqual(line, '1');
break;

case 1:
assert.strictEqual(line, '');
break;

case 2:
assert.strictEqual(line, '2');
break;

default:
assert.fail();
break;
}
}
} finally {
await file?.close();
}