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

Request.body does not seem to interface correctly with ReadableStream #3862

Closed
xdivby0 opened this issue Jul 29, 2022 · 6 comments
Closed

Request.body does not seem to interface correctly with ReadableStream #3862

xdivby0 opened this issue Jul 29, 2022 · 6 comments
Assignees
Labels
bug:unverified feat:fetch Issues related to @remix-run/web-fetch

Comments

@xdivby0
Copy link
Contributor

xdivby0 commented Jul 29, 2022

What version of Remix are you using?

1.6.0

Steps to Reproduce

The request.body seems to be a web compatible ReadableStream, but it does not seem to be 100% compatible yet. Reproduce with this:

import {Readable} from "node:stream";

const stream = Readable.fromWeb(request.body);
const x = await s3.upload({
    Bucket: BUCKET_NAME,
    Key: id,
    Body: stream,
}).promise();

This will yield:

TS2345: Argument of type 'ReadableStream<Uint8Array>' is not assignable to parameter of type 'ReadableStream<any>'.
Type 'ReadableStream<Uint8Array>' is missing the following properties from type 'ReadableStream<any>': values, [Symbol.asyncIterator]

This is not just a TS thing, as casting it will result in runtime error:

TypeError: The "readableStream" argument must be an instance of ReadableStream. Received an instance of ReadableStream
    at new NodeError (node:internal/errors:387:5)
    at Object.newStreamReadableFromReadableStream (node:internal/webstreams/adapters:463:11)
    at Function.Readable.fromWeb (node:internal/streams/readable:1403:27)
    at action (C:\Dateien\webdev\paycheck\app\routes\__loggedIn\projects\$projectId\uploadMedia.ts:49:29)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at Object.callRouteAction (C:\Dateien\webdev\paycheck\node_modules\@remix-run\server-runtime\data.js:40:14)
    at handleResourceRequest (C:\Dateien\webdev\paycheck\node_modules\@remix-run\server-runtime\server.js:442:14)
    at requestHandler (C:\Dateien\webdev\paycheck\node_modules\@remix-run\server-runtime\server.js:42:18)
    at C:\Dateien\webdev\paycheck\node_modules\@remix-run\express\server.js:39:22 {
  code: 'ERR_INVALID_ARG_TYPE'
}

Expected Behavior

I expect request.body to contain an object of type ReadableStream that fulfills all the properties a standardized web ReadableStream has.

I thus expect Readable.fromWeb() to take request.body as a valid parameter.

Actual Behavior

The ReadableStream request.body is not getting accepted by Readable.fromWeb().

@kiliman
Copy link
Collaborator

kiliman commented Jul 29, 2022

Hmm. Interesting. Remix uses this package to support web streams. Which ultimately uses web-streams-polyfill

Here's the line that adds [Symbol.asyncInterator]. I think the issue may be that symbols don't match on name, but by reference. The Readable.fromWeb() is probably returning a different ReadableStream than what Remix uses/expects.

I have a function to convert from node streams to web streams, instead of .fromWeb()

export function nodeStreamToReadableStream(nodeStream: Readable) {
  return new ReadableStream({
    start(controller: any) {
      nodeStream.on('data', (chunk: any) => {
        controller.enqueue(chunk)
      })
      nodeStream.on('end', () => {
        controller.close()
      })
    },
  })
}

@xdivby0
Copy link
Contributor Author

xdivby0 commented Jul 30, 2022

@kiliman I did something similar, I now use

    const bodyIterable: AsyncIterable<any> = {
        [Symbol.asyncIterator]() {
            const reader = request.body?.getReader();
            return {
                async next() {
                    const {done, value} = await reader?.read()??{done: true, value: undefined};
                    return {done, value};
                }
            };
        }
    };
    const stream = Readable.from(bodyIterable, {
        objectMode: false,
        highWaterMark: 16,
    });

This should probably make sure back pressure and stuff like that get respected.

But the actual issue is not that the Readable.fromWeb() is returning something wrong, but instead the req.body is not interfacing correctly with the ReadableStream, or am I wrong?

@titocosta
Copy link

While using the serpapi/serpapi-javascript package (which uses the nodejs/undici HTTP/1.1 client under the hood), imported from a file using the .server. naming convention, i encountered what may be the same problem with the error below.

Ultimately request.body fails the assertReadableStream type check.

/Users/.../node_modules/web-streams-polyfill/src/lib/validators/readable-stream.ts:5
    throw new TypeError(`${context} is not a ReadableStream.`);
          ^
TypeError: First parameter has member 'readable' that is not a ReadableStream.
    at assertReadableStream (/Users/.../node_modules/web-streams-polyfill/src/lib/validators/readable-stream.ts:5:11)
    at convertReadableWritablePair (/Users/.../node_modules/web-streams-polyfill/src/lib/validators/readable-writable-pair.ts:15:3)
    at ReadableStream.pipeThrough (/Users/.../node_modules/web-streams-polyfill/src/lib/readable-stream.ts:211:23)
    at fetchFinale (/Users/.../node_modules/undici/lib/fetch/index.js:973:52)
    at mainFetch (/Users/.../node_modules/undici/lib/fetch/index.js:773:5)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
fatal error: all goroutines are asleep - deadlock!

@jacob-ebey
Copy link
Member

Looks like this isn't going to be possible as they rely on privately defined symbols in the Node internals to check if the thing passed to fromWeb / toWeb is valid (here, and here). We do however provide the following functions from the @remix-run/node package:

  • createReadableStreamFromReadable
  • readableStreamToString,
  • writeAsyncIterableToWritable,
  • writeReadableStreamToWritable,

@ariofrio
Copy link

ariofrio commented Sep 2, 2023

@jacob-ebey Wait, I see that createReadableStreamFromReadable can be used instead of toWeb, but which of those functions acts as a replacement for fromWeb? Seems like it's missing and should be added to @remix-run/node to resolve this issue.

@timothymalcham
Copy link

I've also encountered this issue, and I'm trying to use createReadableStreamFromReadable. Could you provide some guidance @jacob-ebey?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug:unverified feat:fetch Issues related to @remix-run/web-fetch
Projects
No open projects
Status: Closed
Development

No branches or pull requests

8 participants
@kiliman @titocosta @ariofrio @brophdawg11 @timothymalcham @jacob-ebey @xdivby0 and others