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

Single Fetch Issues #9324

Closed
michaelgmcd opened this issue Apr 26, 2024 · 9 comments
Closed

Single Fetch Issues #9324

michaelgmcd opened this issue Apr 26, 2024 · 9 comments

Comments

@michaelgmcd
Copy link
Contributor

michaelgmcd commented Apr 26, 2024

Note: I'm still working on the migration/testing, so I kept this title vague for now in case I come across other problems.

Reproduction

https://stackblitz.com/edit/remix-run-remix-tfwhw2?file=app%2Froutes%2F_index.tsx

System Info

Vite: 5.2.10
Remix: 2.9.1

System:
    OS: macOS 14.4.1
    CPU: (10) arm64 Apple M2 Pro
    Memory: 80.28 MB / 32.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.12.2 - /opt/n/bin/node
    npm: 10.5.0 - /opt/n/bin/npm
    pnpm: 8.15.7 - /opt/homebrew/bin/pnpm
    Watchman: 2024.04.08.00 - /opt/homebrew/bin/watchman
  Browsers:
    Chrome: 124.0.6367.78
    Safari: 17.4.1

Used Package Manager

pnpm

Expected Behavior

Happy to split this out into separate issues, but while migrating to Single Fetch, I've found two issues:

  1. From the Single Fetch overview doc:

If you are currently returning Response instances from your loaders (i.e., json/defer) then you shouldn't need to make many changes to your app code, but please read through the "breaking" changes below to be aware of some of the underlying behavior changes - specifically around serialization and status/header behavior.

This is in the reproduction, but for the existing use case with json(), the application crashes:

export const loader = ({ response }: LoaderFunctionArgs) => {
  return json({}, {
    headers: {
      'X-Test-Header': 'Foo-Bar'
    }
  })
}

json and other utilities now require the headers key to be an instance of Headers.

It could be as simple as checking if result.result.headers instanceof Headers before calling proxyResponseToResponseStub, or making the headers argument of proxyResponseToResponseStub something like headers: Headers | Record<string, string>, and handling it accordingly.

  1. This is half question, half bug report.

From the Single fetch headers doc:

To alter the status of your HTTP Response, set the status field directly:
response.status = 201

However, the response stub is potentially undefined (the type in the doc calls this out as well). When is response stub undefined? Having to use a non-null assertion every time doesn't feel right, and typescript throws The left-hand side of an assignment expression may not be an optional property access. if trying to do something like response?.status = 200

Actual Behavior

  1. The type of json would be correct and disallow plain objects for headers or support them
  2. Clear documentation on when the response stub is undefined or improved ways of handling things like redirects (The doc mentions the current utilities are deprecated)
@michaelgmcd
Copy link
Contributor Author

After looking at this a bit more, even a simple:

export const loader = () => {
  return json({})
}

throws the same error:

TypeError: headers.getSetCookie is not a function or its return value is not iterable
    at proxyResponseToResponseStub (file:///home/projects/remix-run-remix-tfwhw2/node_modules/@remix-run/server-runtime/dist/single-fetch.js:292:25)
    at eval (file:///home/projects/remix-run-remix-tfwhw2/node_modules/@remix-run/server-runtime/dist/single-fetch.js:61:9)
    at async Promise.all (index 1)
    at async eval (file:///home/projects/remix-run-remix-tfwhw2/node_modules/@remix-run/server-runtime/dist/single-fetch.js:39:19)
    at async callDataStrategyImpl (file:///home/projects/remix-run-remix-tfwhw2/node_modules/@remix-run/router/dist/router.cjs.js:4169:17)
    at async callDataStrategy (file:///home/projects/remix-run-remix-tfwhw2/node_modules/@remix-run/router/dist/router.cjs.js:3702:19)
    at async loadRouteData (file:///home/projects/remix-run-remix-tfwhw2/node_modules/@remix-run/router/dist/router.cjs.js:3677:19)
    at async queryImpl (file:///home/projects/remix-run-remix-tfwhw2/node_modules/@remix-run/router/dist/router.cjs.js:3522:20)
    at async Object.query (file:///home/projects/remix-run-remix-tfwhw2/node_modules/@remix-run/router/dist/router.cjs.js:3416:18)
    at async handleDocumentRequest (file:///home/projects/remix-run-remix-tfwhw2/node_modules/@remix-run/server-runtime/dist/server.js:222:15)

@michaelgmcd
Copy link
Contributor Author

michaelgmcd commented Apr 26, 2024

Found the culprit for the first issue. installGlobals without nativeFetch uses remix's own polyfill, which doesn't implement headers.getSetCookie. The docs aren't clear on whether or not installGlobals should be called for custom servers.

@coji
Copy link

coji commented Apr 27, 2024

I am having the same problem with vercel.

Here is my reproduction code.
https://github.com/coji/remix-vercel-single-fetch-test

I was wondering where to put installGlobals since it's not a custom server.

@Tsirimaholy
Copy link

I commented on the Vercel Fork, but I will leave it there in case it may help.
I'm getting this error too. I tried to use node 20 if it solves the issue, but it's the same.
The server is not accessible (Just blank screen).
Mine is just a local dev environment.
I tried to dig deeper a little bit and there I see this line:

for (let v of headers.getSetCookie()) {

Even the comments themselves acknowledge the presence of certain typing issues.
I want to contribute to solve this issue, but i'm not a yet great JS dev myself 😅 .

@brophdawg11
Copy link
Contributor

brophdawg11 commented Apr 27, 2024

Regarding the headers issue - It looks like we may have missed this call out in the docs and only included it in the release notes: https://github.com/remix-run/remix/blob/main/CHANGELOG.md#single-fetch-unstable

Single fetch requires using the built-in fetch on node 20, or passing the new option to installGlobals({ nativeFetch: true }) in your custom server.

I’ll get that added to the docs 👍

Update Added to the docs in #9335

@michaelgmcd
Copy link
Contributor Author

Just came back here to say the same thing. Rereading the release notes and saw it.

@brophdawg11
Copy link
Contributor

To recap here, I think this can be closed out?

  1. was due to a missing installGlobals({ nativeFetch: true }) - we added a call out to the docs inhttps://github.com/Add section on undici polyfill to Single Fetch docs #9335.
  2. is a duplicate of Single fetch response parameter for loader is undefined if no default export exists in the route #9314 and being addressed in Resource Route Single Fetch Updates #9349. We are adding the response stub to resource routes to eliminate the ambiguity, and I think we are also probably going to look into a LoaderFunctionArgs override in the future/single-fetch.d.ts file so we don't have to introduce response as optionl when apps have not opted into single fetch.

Please let me know if anything is still outstanding and we can re-open!

@brophdawg11 brophdawg11 removed their assignment Apr 30, 2024
@sahithyandev
Copy link

I have enabled Single Fetch in my Remix (with Vite) project. It's working locally, but not on Vercel. I am seeing headers.getSetCookie is not defined error, and a blank page.

I am using Node 20. I am calling installGlobals({ nativeFetch: true }) in vite.config.ts

@michalmo
Copy link

There's an issue for the problem with Vercel at vercel#109 where I posted a workaround.

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

No branches or pull requests

6 participants