Skip to content

Commit 11085b9

Browse files
clemyanarcanis
andauthored
PnP: Allow FileHandle#read into TypedArrays and DataViews (#6950)
## What's the problem this PR addresses? Fixes #6890 Fixes #6145 Node's [implementation](https://github.com/nodejs/node/blob/94cbb7758282bf5db837412f7cb3698f17ac1af3/lib/internal/fs/promises.js#L648) of `FileHandle#read` allows reading into `TypedArray`s and `DataView`s (collectively known as `ArrayBufferView`s, and checks for them using `ArrayBuffer.isView()`. On the other hand, our implementation checks using `Buffer.isBuffer()` and thus only allows `Buffer`. If an `ArrayBufferView` is given, it treats them as an options bag and pass its `.buffer` (which is an `ArrayBuffer`) as the sink to the backing fs. If the backing fs is Node's native `fs` then it would throw. ## How did you fix it? Allow `ArrayBufferView`s by checking with `ArrayBuffer.isView()` as in Node's implementation. Because the underlying `FakeFS` interface only allows `Buffer` (and changing that would be a breaking change), a `Buffer` backed by the `ArrayBufferView` is used. (Q: Should we make that change next major?) Also un-vendored the TypeScript types if the exact same type is available from `@types/node` along the way. AFAICT those were vendored because those types wasn't available at first? \cc @merceyz This fix should be orthogonal to #6919 ## Checklist <!--- Don't worry if you miss something, chores are automatically tested. --> <!--- This checklist exists to help you remember doing the chores when you submit a PR. --> <!--- Put an `x` in all the boxes that apply. --> - [x] I have read the [Contributing Guide](https://yarnpkg.com/advanced/contributing). <!-- See https://yarnpkg.com/advanced/contributing#preparing-your-pr-to-be-released for more details. --> <!-- Check with `yarn version check` and fix with `yarn version check -i` --> - [x] I have set the packages that need to be released for my changes to be effective. <!-- The "Testing chores" workflow validates that your PR follows our guidelines. --> <!-- If it doesn't pass, click on it to see details as to what your PR might be missing. --> - [x] I will check that all automated PR checks pass before the PR gets reviewed. --------- Co-authored-by: Maël Nison <[email protected]>
1 parent a592371 commit 11085b9

File tree

7 files changed

+302
-110
lines changed

7 files changed

+302
-110
lines changed

.pnp.cjs

100755100644
Lines changed: 22 additions & 10 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

.yarn/versions/21efef87.yml

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
releases:
2+
"@yarnpkg/cli": patch
3+
"@yarnpkg/fslib": patch
4+
"@yarnpkg/pnp": patch
5+
6+
declined:
7+
- "@yarnpkg/plugin-catalog"
8+
- "@yarnpkg/plugin-compat"
9+
- "@yarnpkg/plugin-constraints"
10+
- "@yarnpkg/plugin-dlx"
11+
- "@yarnpkg/plugin-essentials"
12+
- "@yarnpkg/plugin-exec"
13+
- "@yarnpkg/plugin-file"
14+
- "@yarnpkg/plugin-git"
15+
- "@yarnpkg/plugin-github"
16+
- "@yarnpkg/plugin-init"
17+
- "@yarnpkg/plugin-interactive-tools"
18+
- "@yarnpkg/plugin-jsr"
19+
- "@yarnpkg/plugin-link"
20+
- "@yarnpkg/plugin-nm"
21+
- "@yarnpkg/plugin-npm"
22+
- "@yarnpkg/plugin-npm-cli"
23+
- "@yarnpkg/plugin-pack"
24+
- "@yarnpkg/plugin-patch"
25+
- "@yarnpkg/plugin-pnp"
26+
- "@yarnpkg/plugin-pnpm"
27+
- "@yarnpkg/plugin-stage"
28+
- "@yarnpkg/plugin-typescript"
29+
- "@yarnpkg/plugin-version"
30+
- "@yarnpkg/plugin-workspace-tools"
31+
- vscode-zipfs
32+
- "@yarnpkg/builder"
33+
- "@yarnpkg/core"
34+
- "@yarnpkg/doctor"
35+
- "@yarnpkg/libzip"
36+
- "@yarnpkg/nm"
37+
- "@yarnpkg/pnpify"
38+
- "@yarnpkg/sdks"
39+
- "@yarnpkg/shell"

packages/acceptance-tests/pkg-tests-specs/sources/commands/patchCommit.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ describe(`Commands`, () => {
114114
});
115115

116116
expect(manifest.resolutions).toEqual({
117-
[`no-deps@npm:1.0.0`]: expect.stringMatching(/^patch:no-deps/),
117+
[`no-deps@npm:1.0.0`]: expect.stringMatching(/^patch:no-deps/),
118118
});
119119
}),
120120
);

packages/yarnpkg-fslib/sources/patchFs/FileHandle.ts

Lines changed: 65 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -1,49 +1,23 @@
1-
import type {BigIntStats, ReadStream, StatOptions, Stats, WriteStream, WriteVResult} from 'fs';
2-
import {createInterface} from 'readline';
3-
4-
import type {CreateReadStreamOptions, CreateWriteStreamOptions, FakeFS} from '../FakeFS';
5-
import type {Path} from '../path';
6-
7-
// Types copied from https://github.com/DefinitelyTyped/DefinitelyTyped/blob/9e2e5af93f9cc2cf434a96e3249a573100e87351/types/node/v16
8-
// Implementation based on https://github.com/nodejs/node/blob/10493b48c7edb227c13a493d0a2c75efe878d7e9/lib/internal/fs/promises.js#L124-L336
9-
10-
interface ObjectEncodingOptions {
11-
encoding?: BufferEncoding | null | undefined;
12-
}
13-
14-
interface FlagAndOpenMode {
15-
mode?: Mode | undefined;
16-
flag?: OpenMode | undefined;
17-
}
18-
19-
type OpenMode = number | string;
20-
type Mode = number | string;
21-
22-
interface FileReadResult<T extends ArrayBufferView> {
23-
bytesRead: number;
24-
buffer: T;
25-
}
26-
27-
interface FileReadOptions<T extends ArrayBufferView = Buffer> {
28-
buffer?: T;
29-
offset?: number | null;
30-
length?: number | null;
31-
position?: number | null;
32-
}
33-
34-
interface ReadVResult {
35-
bytesRead: number;
36-
buffers: Array<NodeJS.ArrayBufferView>;
37-
}
38-
39-
interface AbortSignal {
40-
readonly aborted: boolean;
41-
}
42-
43-
interface Abortable {
44-
signal?: AbortSignal | undefined;
45-
}
46-
1+
import type {Abortable} from 'events';
2+
import type {FlagAndOpenMode, FileReadResult, FileReadOptions} from 'fs/promises';
3+
import {createInterface} from 'readline';
4+
5+
import type {CreateReadStreamOptions, CreateWriteStreamOptions, FakeFS} from '../FakeFS';
6+
import type {Path} from '../path';
7+
8+
import type {
9+
BigIntStats,
10+
ObjectEncodingOptions,
11+
OpenMode,
12+
ReadStream,
13+
ReadVResult,
14+
StatOptions,
15+
Stats,
16+
WriteStream,
17+
WriteVResult,
18+
} from 'fs';
19+
20+
// Implementation based on https://github.com/nodejs/node/blob/v18.12.0/lib/internal/fs/promises.js#L132-L351
4721

4822
type WriteArgsBuffer<TBuffer extends Uint8Array> = [
4923
buffer: TBuffer,
@@ -133,45 +107,70 @@ export class FileHandle<P extends Path> {
133107
throw new Error(`Method not implemented.`);
134108
}
135109

136-
async read(options?: FileReadOptions<Buffer>): Promise<FileReadResult<Buffer>>;
137-
async read(
138-
buffer: Buffer,
110+
// TODO: Once we drop Node 20 support, switch to ReadOptions and ReadOptionsWithoutBuffer from `@types/node`
111+
async read<T extends NodeJS.ArrayBufferView>(
112+
buffer: T,
139113
offset?: number | null,
140114
length?: number | null,
141-
position?: number | null
142-
): Promise<FileReadResult<Buffer>>;
115+
position?: number | null,
116+
): Promise<FileReadResult<T>>;
117+
async read<T extends NodeJS.ArrayBufferView>(
118+
buffer: T,
119+
options?: Omit<FileReadOptions<T>, `buffer`>,
120+
): Promise<FileReadResult<T>>;
121+
async read<T extends NodeJS.ArrayBufferView = NonSharedBuffer>(
122+
options: FileReadOptions<T> & {buffer: T},
123+
): Promise<FileReadResult<T>>;
143124
async read(
144-
bufferOrOptions?: Buffer | FileReadOptions<Buffer>,
145-
offset?: number | null,
125+
options?: FileReadOptions<NonSharedBuffer> & {buffer?: never},
126+
): Promise<FileReadResult<NonSharedBuffer>>;
127+
async read<T extends NodeJS.ArrayBufferView>(
128+
bufferOrOptions?: T | FileReadOptions<T>,
129+
offsetOrOptions?: number | null | Omit<FileReadOptions<T>, `buffer`>,
146130
length?: number | null,
147131
position?: number | null,
148-
): Promise<FileReadResult<Buffer>> {
132+
): Promise<FileReadResult<T>> {
149133
try {
150134
this[kRef](this.read);
151135

152-
let buffer: Buffer;
153-
154-
if (!Buffer.isBuffer(bufferOrOptions)) {
155-
bufferOrOptions ??= {};
156-
buffer = bufferOrOptions.buffer ?? Buffer.alloc(16384);
157-
offset = bufferOrOptions.offset || 0;
158-
length = bufferOrOptions.length ?? buffer.byteLength;
159-
position = bufferOrOptions.position ?? null;
136+
let buffer: T;
137+
let offset: number;
138+
139+
if (!ArrayBuffer.isView(bufferOrOptions)) {
140+
// read([options])
141+
// TypeScript isn't able to infer that the coalescing happens only in the no-generic case
142+
buffer = bufferOrOptions?.buffer ?? Buffer.alloc(16384) as unknown as T;
143+
offset = bufferOrOptions?.offset ?? 0;
144+
length = bufferOrOptions?.length ?? buffer.byteLength - offset;
145+
position = bufferOrOptions?.position ?? null;
146+
} else if (typeof offsetOrOptions === `object` && offsetOrOptions !== null) {
147+
// read(buffer[, options])
148+
buffer = bufferOrOptions;
149+
offset = offsetOrOptions?.offset ?? 0;
150+
length = offsetOrOptions?.length ?? buffer.byteLength - offset;
151+
position = offsetOrOptions?.position ?? null;
160152
} else {
153+
// read(buffer, offset[, length[, position]])
161154
buffer = bufferOrOptions;
155+
offset = offsetOrOptions ?? 0;
156+
length ??= 0;
162157
}
163158

164-
offset ??= 0;
165-
length ??= 0;
166-
167159
if (length === 0) {
168160
return {
169161
bytesRead: length,
170162
buffer,
171163
};
172164
}
173165

174-
const bytesRead = await this[kBaseFs].readPromise(this.fd, buffer, offset, length, position);
166+
const bytesRead = await this[kBaseFs].readPromise(
167+
this.fd,
168+
// FIXME: FakeFS should support ArrayBufferViews directly
169+
Buffer.isBuffer(buffer) ? buffer : Buffer.from(buffer.buffer, buffer.byteOffset, buffer.byteLength),
170+
offset,
171+
length,
172+
position,
173+
);
175174

176175
return {
177176
bytesRead,
154 Bytes
Binary file not shown.

0 commit comments

Comments
 (0)