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

Error: Expected a Response to be returned from queryRoute #4741

Closed
alfonsocartes opened this issue Dec 2, 2022 · 19 comments · Fixed by #4782
Closed

Error: Expected a Response to be returned from queryRoute #4741

alfonsocartes opened this issue Dec 2, 2022 · 19 comments · Fixed by #4782
Labels
bug Something isn't working package:server-runtime

Comments

@alfonsocartes
Copy link

alfonsocartes commented Dec 2, 2022

What version of Remix are you using?

1.8.0

Steps to Reproduce

I'm downloading an image from S3 with a resource route and I get Error: Expected a Response to be returned from queryRoute.

This doesn't happen in 1.7.6.

Expected Behavior

Get image

Actual Behavior

GET /assets/images/[AWSIMAGEKEY]?w=768&format=webp 500 - - 563.637 ms
The following error is a bug in Remix; please open an issue! https://github.com/remix-run/remix/issues/new
Error: Expected a Response to be returned from queryRoute
    at Object.invariant [as default] (/Users/.../node_modules/@remix-run/server-runtime/dist/invariant.js:18:11)
    at handleResourceRequestRR (/Users/.../node_modules/@remix-run/server-runtime/dist/server.js:267:25)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at requestHandler (/Users/.../node_modules/@remix-run/server-runtime/dist/server.js:46:18)
    at /Users/.../node_modules/@remix-run/express/dist/server.js:39:22
@brophdawg11
Copy link
Contributor

@alfonsocartes Are you able to provide a minimal reproduction or code sample of what your loader looks like and how you're getting the image from S3? That would help us dig into the root cause here. Thanks!

@alfonsocartes
Copy link
Author

alfonsocartes commented Dec 2, 2022

Sure, the loader fetches from an external API through a resource route.

It's based on this example: https://github.com/remix-run/examples/tree/main/file-and-s3-upload

export async function loader({ request, params }: LoaderArgs) {
  try {
    // extract all the parameters from the url
    const { key, width, height, fit, format } = extractParams(params, request);

    if (
      (width && !ALLOWED_IMAGE_SIZES.includes(width)) ||
      (height && !ALLOWED_IMAGE_SIZES.includes(height))
    ) {
      throw new Error(`Image size not allowed`);
    }
    return downloadImage(key, width, height, fit, format);
  } catch (error: unknown) {
    // if the image is not found, or we get any other errors we return different response types
    return handleError(error);
  }
}

The downloadImage function calls an external API:

return fetch(
      `${process.env.API_URL}/images/${imageKey}?width=${width}&height=${height}&fit=${fit}&format=${format}`
    );

And the external API streams the image:

router.get("/:imageKey", async (req, res) => {
  try {
    const { imageKey } = req.params;
    const { width, height, fit, format } = req.query;

    const w =
      (typeof width === "string" && Number.parseInt(width)) || undefined;
    const h =
      (typeof height === "string" && Number.parseInt(height)) || undefined;
    const f = fit != "undefined" ? (fit as keyof sharp.FitEnum) : undefined;
    const type =
      format != "undefined" ? (format as keyof sharp.FormatEnum) : undefined;

    if (
      w &&
      typeof w === "string" &&
      !ALLOWED_IMAGE_SIZES.includes(Number.parseInt(w))
    ) {
      return res.status(400).send("Invalid width size");
    }

    if (
      h &&
      typeof h === "string" &&
      !ALLOWED_IMAGE_SIZES.includes(Number.parseInt(h))
    ) {
      return res.status(400).send("Invalid height size");
    }

    // Download image from S3
    const readStream = await downloadFileFromS3(imageKey);

    if (!readStream) {
      return res.status(404).send("Image not found");
    }

    // Resize image accourding to query params
    const resizedStream = streamingResize(readStream, w, h, f, type);

    // Send the image to the client with the correct content type and cache control
    const resHeaders = {
      "Content-Type": `image/${type ?? "jpeg"}`,
      "Cache-Control": "public, max-age=31536000, inmutable",
    };

    res.writeHead(200, resHeaders);

    // This sends the stream straight to the client.
    resizedStream.pipe(res);
  } catch (error) {
    console.error("POST /images/", error);

    const resHeaders = {
      "Content-Type": "text/plain",
      "Cache-Control": "no-cache, no-store, must-revalidate",
    };
    res.set(resHeaders);

    // error needs to be typed
    const errorT = error as Error & { code: string };
    // if the read stream fails, it will have the error.code ENOENT
    if (errorT.code === "ENOENT") {
      return res.status(404).json({ message: "Could not download image" });
    }
    // if there is an error processing the image, we return a 500 error
    return res.status(500).json({ message: errorT.message });
  }
});
export async function downloadFileFromS3(Key: string) {
  const s3Client = new S3Client({
    credentials: {
      accessKeyId: AWS_ACCESS_KEY!,
      secretAccessKey: AWS_SECRET!,
    },
    region: STORAGE_REGION,
  });

  const downloadParams = {
    // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
    Bucket: STORAGE_NAME!,
    Key,
  };

  // return s3Client.getObject(downloadParams).createReadStream();
  const s3Item = await s3Client.send(new GetObjectCommand(downloadParams));
  console.info(
    "Successfully downloaded " +
      downloadParams.Key +
      " from " +
      downloadParams.Bucket +
      "/" +
      downloadParams.Key
  );

  if (s3Item.Body) {
    const stream = s3Item.Body as Readable;
    return stream;
  } else {
    throw new Error("No body");
  }
}

Thank you @brophdawg11

@brophdawg11 brophdawg11 self-assigned this Dec 2, 2022
@leimantas
Copy link

Have same problem with oracle. return fetch(oracle_url, { headers })

@leimantas
Copy link

leimantas commented Dec 3, 2022

Error: Expected a Response to be returned from guervRoute
at Object.invariant [as default] (/website/node_modules/@remix-run/server-runtime/dist/invariant.js:18:11)
at handleResourceRequestRR (/website/node modules/@remix-run/server-runtime/dist/server.is:267:25)
at runMicrotasks (<anonvmous>)
at processTicksAndRejections (node: internal/process/task queues: 96:5 )
at requestHandler(/website/node_modules/@remix-run/server-runtime/dist/server.js:46:18)
at /website/node modules/@remix-run/express/dist/server.is:39:22
GET /assets/images/post content bq. ipg?blur=2 500 - - 2041.300 ms```

@leimantas
Copy link

leimantas commented Dec 6, 2022

import type { LoaderArgs } from '@remix-run/node';

export async function loader(_args: LoaderArgs) {
  return fetch('https://google.com');
}

Remix 1.8.1
image
Result:
image

Remix 1.7.6
image
Result:
image

Theres something with FETCH in latest version.

@brophdawg11
Copy link
Contributor

Thank you! This simple reproduction will be a big help. My initial hunch is that it's something with our instanceof Response check and we may have multiple Response classes defined. Planning to dig in today 👍

@kentcdodds
Copy link
Member

Heya, I just bumped into this. I can confirm that it's response instances that are the problem. I added this console.log to handleResourceRequestRR:

    console.log({
      globalResponse: Response,
      remixResponse: webFetch.Response,
      responseIsGlobal: response instanceof Response,
      responseIsRemix: response instanceof webFetch.Response,
      responsesAreSame: Response === webFetch.Response,
    });

Here's what I got:

{
  globalResponse: [class NodeResponse extends Response],
  remixResponse: [class Response extends Body],
  responseIsGlobal: false,
  responseIsRemix: true,
  responsesAreSame: false
}

instanceof checks are always going to be problematic I think. People may want to use node-fetch for whatever reason (maybe a client library they're using uses it) and they'll be surprised/disappointed if this doesn't "just work" because they will then have to figure out how to convert the node-fetch response to a web fetch response.

Any instanceof will need to change to a isResponseLike call or something.

@kentcdodds
Copy link
Member

Oh, and in my case I'm simply returning fetch from my loader. Interestingly whether I use global fetch or the one from @remix-run/web-fetch, the response is a Remix Response.

So it appears that somehow the global Node Response isn't getting overridden by the Remix Response for handleResourceRequestRR 🤷‍♂️

@brophdawg11
Copy link
Contributor

Yeah this boils down to the CJS Response class being used by your node server versus the ESM Response class being returned from the loader in your compiled bundle. We just released the fix for this in a prerelease (1.8.2-pre.0) if folks want to give that a shot and confirm it fixes the issues!

@leimantas
Copy link

Yeah this boils down to the CJS Response class being used by your node server versus the ESM Response class being returned from the loader in your compiled bundle. We just released the fix for this in a prerelease (1.8.2-pre.0) if folks want to give that a shot and confirm it fixes the issues!

All good! It works now.

@johnhayde
Copy link

We're still seeing this error after updating to 1.8.2-pre.0 stack trace below:

Error: Expected a Response to be returned from queryRoute
    at Object.invariant [as default] (/var/task/node_modules/@remix-run/server-runtime/dist/invariant.js:18:11)
    at handleResourceRequestRR (/var/task/node_modules/@remix-run/server-runtime/dist/server.js:267:25)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at requestHandler (/var/task/node_modules/@remix-run/server-runtime/dist/server.js:46:18)
    at Runtime.handler (/var/task/node_modules/@remix-run/architect/dist/server.js:39:20)

We're calling Google Maps API and getting a 200 back, then get the above error. Sample snippet below:

import type { LoaderFunction } from "@remix-run/node";
import contentfulKeys from "~/utils/config";

export const loader: LoaderFunction = async ({ params }) => {
  const val = params.val;
  const autocomplete = await fetch(
    `https://maps.googleapis.com/maps/api/place/autocomplete/json?input=${val}&key=${contentfulKeys.googleApiKey}&components=country:us`
  )
    .then((response: any) => {
      return response;
    })
    .catch((error) => {
      console.log("Error returned from Google Maps API Service Call: ",error)
      return error;
    });
  console.log("Returned value from Google Maps API Service Call: ", autocomplete);
  return autocomplete;
};

@brophdawg11
Copy link
Contributor

@johnhayde Any chance you could look at whats returned from your loader there? Does it satisfy the check we're using for isResponse?

function isResponse(value: any): value is Response {
  return (
    value != null &&
    typeof value.status === "number" &&
    typeof value.statusText === "string" &&
    typeof value.headers === "object" &&
    typeof value.body !== "undefined"
  );
}

@johnhayde
Copy link

johnhayde commented Dec 6, 2022

@brophdawg11 Here's the response object we're getting from our loader:

Response {
   size: 0,
   [Symbol(Body internals)]: {
     body: ReadableStream {
       _state: 'readable',
       _reader: undefined,
       _storedError: undefined,
       _disturbed: false,
       _readableStreamController: [ReadableStreamDefaultController]
     },
     type: null,
     size: null,
     boundary: null,
     disturbed: false,
     error: null
   },
   [Symbol(Response internals)]: {
     url: 'https://maps.googleapis.com/maps/api/place/autocomplete/json?input=a&key=<api-key>&components=country:us',
     status: 200,
     statusText: 'OK',
     headers: {
       'accept-ranges': 'none',
       'alt-svc': 'h3=":443"; ma=2592000,h3-29=":443"; ma=2592000,h3-Q050=":443"; ma=2592000,h3-Q046=":443"; ma=2592000,h3-Q043=":443"; ma=2592000,quic=":443"; ma=2592000; v="46,43"',
       'cache-control': 'private, max-age=300',
       connection: 'close',
       'content-type': 'application/json; charset=UTF-8',
       date: 'Tue, 06 Dec 2022 23:05:13 GMT',
       expires: 'Tue, 06 Dec 2022 23:05:13 GMT',
       server: 'scaffolding on HTTPServer2',
       'server-timing': 'gfet4t7; dur=21',
       'transfer-encoding': 'chunked',
       vary: 'Accept-Language,Accept-Encoding',
       'x-frame-options': 'SAMEORIGIN',
       'x-xss-protection': '0'
     },
     counter: 0,
     highWaterMark: 16384
   }
 }

@johnhayde
Copy link

@brophdawg11 downgrading to 1.7.6 was not working for us either. We changed calls from fetch to axios and passed response.data between calls instead of the whole response object. This resolved the issue.

@alfonsocartes
Copy link
Author

@brophdawg11 1.8.2-pre.0 works! it fixed the issue. Thanks Matt.

@brophdawg11
Copy link
Contributor

@johnhayde Huh - interesting. Glad it's working for you, but definitely curious why fetch wouldn't work directly - it should! Maybe there was something weird going on in architect there...

@brophdawg11 brophdawg11 added bug Something isn't working awaiting release This issue has been fixed and will be released soon package:server-runtime and removed bug:unverified labels Dec 7, 2022
@benbrandt
Copy link

@brophdawg11 was also seeing this behavior returning from fetch directly. Even when I specifically imported fetch from @remix-run/web-fetch, the instanceof check for Response failed in the loader.

As in

const response = await fetch("someurl");

console.log(response instanceof Response); // false

@brophdawg11
Copy link
Contributor

This is released in 1.8.2

@benbrandt That instanceof check in user code has come up before - let's follow that over here: #3480

@kentcdodds
Copy link
Member

FYI, this update works for me 👍 Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working package:server-runtime
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants