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

cannot return HTTP 304 using unstable_singleFetch #9930

Closed
linobino1 opened this issue Sep 1, 2024 · 10 comments · Fixed by #9941
Closed

cannot return HTTP 304 using unstable_singleFetch #9930

linobino1 opened this issue Sep 1, 2024 · 10 comments · Fixed by #9941

Comments

@linobino1
Copy link

Reproduction

clone https://github.com/linobino1/remix-mvce-304 and run:

npm i
npm run dev

# In another terminal
curl -H "If-None-Match: 1234" http://localhost:5173/_root.data

you will see:

[vite] Internal server error: Response constructor: Invalid response status code 304
      at webidl.errors.exception (node:internal/deps/undici/undici:2414:14)
      at initializeResponse (node:internal/deps/undici/undici:6242:31)
      at new Response (node:internal/deps/undici/undici:6034:9)
      at handleSingleFetchRequest (/Users/leohilsheimer/Sites/remix-mvce-304/node_modules/@remix-run/server-runtime/dist/server.js:254:10)
      at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
      at async requestHandler (/Users/leohilsheimer/Sites/remix-mvce-304/node_modules/@remix-run/server-runtime/dist/server.js:133:18)
      at async nodeHandler (/Users/leohilsheimer/Sites/remix-mvce-304/node_modules/@remix-run/dev/dist/vite/plugin.js:842:27)
      at async /Users/leohilsheimer/Sites/remix-mvce-304/node_modules/@remix-run/dev/dist/vite/plugin.js:845:15

System Info

System:
    OS: macOS 14.6.1
    CPU: (8) arm64 Apple M1 Pro
    Memory: 124.95 MB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 21.7.1 - /opt/homebrew/bin/node
    Yarn: 1.22.21 - /opt/homebrew/bin/yarn
    npm: 10.5.0 - /opt/homebrew/bin/npm
    pnpm: 8.15.7 - /opt/homebrew/bin/pnpm
  Browsers:
    Brave Browser: 124.1.65.133
    Chrome: 128.0.6613.86
    Edge: 128.0.2739.54
    Safari: 17.6
  npmPackages:
    @remix-run/dev: ^2.11.2 => 2.11.2 
    @remix-run/node: ^2.11.2 => 2.11.2 
    @remix-run/react: ^2.11.2 => 2.11.2 
    @remix-run/serve: ^2.11.2 => 2.11.2 
    vite: ^5.1.0 => 5.4.2

Used Package Manager

npm

Expected Behavior

I would expect an empty Response with a status of 304 being returned.

Actual Behavior

The server throws an error Invalid response status code 304.

@linobino1
Copy link
Author

I am also wondering why Response is taken from the undici polyfill... Shouldn't it use the native Web Fetch API Response?
I also tried with a custom express server via https://github.com/kiliman/remix-express-vite-plugin but it's still using the polyfill.

@brophdawg11
Copy link
Contributor

Can you explain your use case for needing a 304? The underlying single fetch fetch call is managed by Remix so there's currently no way to add an If-None-Match header. Returning a 304 for a single fetch response is also a bit odd since it's for multiple routes, not just one so what if the other routes do need their new data returned?

@linobino1
Copy link
Author

I was trying to cache that loader response in a CDN, and revalidate via ETag. Am I missing something fundamental?

@brophdawg11
Copy link
Contributor

brophdawg11 commented Sep 3, 2024

Ah ok sorry I thought you were trying to do that programmatically for a single route you could hit directly via .data and was confused. Yeah if your CDN handles that for you then it should be ok. It looks like the error is because there's a body being assigned with a status of 304 which is invalid, so we need to detect the 304 and avoid encoding anything into the body. I'll get a fix in for that shortly 👍

I am also wondering why Response is taken from the undici polyfill

This isn't coming from the polyfill - Node uses undici as it's native fetch implementation, so the stack trace is coming from the internal version of undici included with node I believe:

TypeError: Response constructor: Invalid response status code 304
    at webidl.errors.exception (node:internal/deps/undici/undici:3384:14)
    at initializeResponse (node:internal/deps/undici/undici:9048:31)
    at new Response (node:internal/deps/undici/undici:8822:9)

@brophdawg11 brophdawg11 self-assigned this Sep 3, 2024
@linobino1
Copy link
Author

detect the 304 and avoid encoding anything into the body

Perfect!

This isn't coming from the polyfill - Node uses undici as it's native fetch implementation, so the stack trace is coming from the internal version of undici included with node I believe:

Of course, thanks for the clarification. Undici is in the node depencies: https://github.com/nodejs/node/tree/main/deps/undici

@brophdawg11 brophdawg11 linked a pull request Sep 5, 2024 that will close this issue
@brophdawg11 brophdawg11 added the awaiting release This issue has been fixed and will be released soon label Sep 5, 2024
@brophdawg11 brophdawg11 removed their assignment Sep 5, 2024
Copy link
Contributor

github-actions bot commented Sep 5, 2024

🤖 Hello there,

We just published version 2.12.0-pre.0 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@linobino1
Copy link
Author

It works thanks! But maybe you should do the same for the route itself? Now the .data route can return a 304 but the route itself will still throw the same error.

On a side note, caching loader responses in singleFetch is very dangerous, if authentication data is returned from a parent route's loader;)

@brophdawg11
Copy link
Contributor

True - but that's also always been the case for document requests if your loaders have authentication data in them. Single Fetch is aligning document and data requests closer to one another.

Copy link
Contributor

github-actions bot commented Sep 9, 2024

🤖 Hello there,

We just published version 2.12.0 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@brophdawg11 brophdawg11 removed the awaiting release This issue has been fixed and will be released soon label Sep 9, 2024
@rjaguilar
Copy link

I still get this error in Remix 2.14.0 with v3_singleFetch: true @brophdawg11 :

TypeError: Response constructor: Invalid response status code 304

in my entry.server.tsx

export const handleDataRequest: HandleDataRequestFunction = async (
  response: Response,
  { request },
) => {

  const isGet = methodIsGet(request);
  if (isGet) {
    response.headers.set('etag', etag(body));

    if (request.headers.get('If-None-Match') === response.headers.get('ETag')) {
      return new Response(null, { status: 304, headers: response.headers });
    }
  }

  return response;
};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants