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.

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

Fix readable/writable file stream examples #1191

Merged
merged 2 commits into from
Nov 29, 2021
Merged

Fix readable/writable file stream examples #1191

merged 2 commits into from
Nov 29, 2021

Conversation

dubiousjim
Copy link
Contributor

@dubiousjim dubiousjim commented Nov 28, 2021

These examples weren't closing the filehandle correctly (they called close on a nonexistent fd variable, rather than calling the fileHandle.close method).

Also the ReadableStream example misused the fileHandle.read call: it was supplying an ArrayBuffer and v's byteOffset within the ArrayBuffer, but the API requires supplying v itself and an offset relative to v's start.

Also added the file offset position to that fileHandle.read call. There's no reason otherwise for the example to keep track of this. Alternatively, the argument could be supplied as null, and we just rely on implicit file positioning.


Preview | Diff

These examples weren't closing the filehandle correctly (they called `close` on a nonexistent `fd` variable, rather than calling the `fileHandle.close` method).

Also the ReadableStream example misused the `fileHandle.read` call: it was supplying an ArrayBuffer and v's byteOffset within the ArrayBuffer, but the API requires supplying v itself and an offset relative to v's start.

Also added the file offset `position` to that `fileHandle.read` call. There's no reason otherwise for the example to keep track of this. Alternatively, the argument could be supplied as null, and we just rely on implicit file positioning.
@MattiasBuelens
Copy link
Collaborator

Thanks! Looks good. 👍

Also the ReadableStream example misused the fileHandle.read call: it was supplying an ArrayBuffer and v's byteOffset within the ArrayBuffer, but the API requires supplying v itself and an offset relative to v's start.

I noticed a similar mistake in the "A readable stream with an underlying pull source" example. We're passing an ArrayBuffer to fileHandle.read(), but it only accepts Buffer | TypedArray | DataView.

streams/index.bs

Lines 7305 to 7307 in 5885d94

const buffer = new ArrayBuffer(CHUNK_SIZE);
const { bytesRead } = await fileHandle.read(buffer, 0, CHUNK_SIZE, position);

We should probably use new Uint8Array(CHUNK_SIZE) instead, and use .subarray(0, bytesRead) (or .slice) in the controller.enqueue() call. Could you fix that too as part of this PR? 🙂

@MattiasBuelens MattiasBuelens merged commit 44190d1 into whatwg:main Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants