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

Enable installGlobals({ nativeFetch: true }) when singleFetch is active #122

Closed
coji opened this issue Jul 4, 2024 · 3 comments
Closed

Comments

@coji
Copy link

coji commented Jul 4, 2024

Issue Description

Since Remix 2.9, the singleFetch option has been introduced. When this option is enabled, it requires installGlobals({ nativeFetch: true }) to be set in the runtime for proper functionality. However, the current implementation of @vercel/remix doesn't allow for this configuration, causing issues when deploying to Vercel.

Current Behavior

  1. The singleFetch option works locally but fails when deployed to Vercel.
  2. When using Vercel AI SDK for streaming in a resource route action, the lack of installGlobals({nativeFetch:true}) causes the deployment to fail on Vercel (while working locally). ref: TypeError: headers.getSetCookie is not a function #109
  3. @vercel/remix has hardcoded installGlobals(); in globals.ts on the server side, preventing application-level configuration: https://github.com/vercel/remix/blob/main/packages/vercel-remix/globals.ts#L2

Workaround

Currently, we're applying a patch to server.js using pnpm patch to enable the required functionality: https://github.com/techtalkjp/techtalk.jp/pull/27/files

However, this workaround requires updating the patch every time the package version changes, which is cumbersome and not sustainable.

Proposed Solution

We request a modification to @vercel/remix that automatically enables installGlobals({ nativeFetch: true }) when singleFetch is active during server execution. Specifically:

  1. Detect when singleFetch is enabled in the Remix configuration.
  2. If singleFetch is active, use installGlobals({ nativeFetch: true }) instead of the current hardcoded installGlobals();.

This change would resolve the current issues and prepare for future compatibility, especially considering the direction of Remix v3.

Future Considerations

Given that Remix v3 (with React Router v7) is expected to have singleFetch enabled by default, the current implementation of @vercel/remix will likely need to change regardless. This issue presents an opportunity to address both current and future needs.

Questions

  1. Is there a plan to implement this automatic nativeFetch: true setting when singleFetch is active in @vercel/remix?
  2. If so, what is the timeline for this implementation?
  3. In the meantime, is there a recommended approach to enable these features without resorting to patching?

Thank you for your attention to this matter. We look forward to your response and any potential solutions.

@coji
Copy link
Author

coji commented Jul 5, 2024

I apologize for the confusion in my original post. After further investigation, I've discovered some new information that I'd like to share as an update to this issue:

  1. The patch I mentioned earlier was not actually applied successfully. This explains why the initial solution didn't work as expected.

  2. Upon further investigation of the Vercel Remix builder source code, I discovered an undocumented environment variable VERCEL_REMIX_NATIVE_FETCH that appears to control the nativeFetch behavior. https://github.com/vercel/vercel/blob/main/packages/remix/defaults/server-node.mjs#L10

  3. I tried setting VERCEL_REMIX_NATIVE_FETCH=1 in the Vercel environment variables. This seemed to enable the use of undici for fetch operations, which was the intended behavior.

  4. However, this change led to a new error in Vercel Serverless Functions:

    TypeError: RequestInit: duplex option is required when sending a body.
    

    The full error stack trace is as follows:

    TypeError: RequestInit: duplex option is required when sending a body.
      at new Request (/var/task/node_modules/.pnpm/[email protected]/node_module
    s/undici/lib/web/fetch/request.js:539:15)
      at createRemixRequest (file:///var/task/build/server/nodejs-eyJydW50aW1
    lIjoibm9kZWpzMTRfeCIsInJlZ2lvbiI6InN6LWRhMDEiLCJpc0RldiI6ZmFsc2V9/server-index.mjs:61:10)
      at Server.default (file:///var/task/build/server/nodejs-eyJydW50aW1lIjo
    ibm9kZWpzMTRfeCIsInJlZ2lvbiI6InN6LWRhMDEiLCJpc0RldiI6ZmFsc2V9/server-index.mjs:80:19)
      at /opt/rust/nodejs.js:7:3792
      at AsyncLocalStorage.run (node:async_hooks:346:14)
      at /opt/rust/nodejs.js:7:3780
      at AsyncLocalStorage.run (node:async_hooks:346:14)
      at Server.<anonymous> (/opt/rust/nodejs.js:7:3768)
      at Server.s (/opt/rust/nodejs.js:7:825)
      at Server.emit (node:events:519:28)
    

Given these new findings, I have some additional questions:

  1. Can you confirm the functionality and intended use of the VERCEL_REMIX_NATIVE_FETCH environment variable?
  2. Are there plans to document this variable and its usage in the official @vercel/remix documentation?
  3. Is this new error (related to the duplex option) a known issue when using VERCEL_REMIX_NATIVE_FETCH=1 in Vercel Serverless Functions?
  4. Are there any additional configuration steps required when enabling VERCEL_REMIX_NATIVE_FETCH to ensure compatibility with Vercel Serverless Functions?

I apologize again for any confusion caused by my initial post. I hope this additional information helps in addressing the underlying issues. Thank you for your continued support and attention to this matter.

@TooTallNate
Copy link
Member

  1. Can you confirm the functionality and intended use of the VERCEL_REMIX_NATIVE_FETCH environment variable?

VERCEL_REMIX_NATIVE_FETCH env var is indeed for the purpose of setting nativeFetch: true. Feel free to use it today.

  1. Are there plans to document this variable and its usage in the official @vercel/remix documentation?

To be determined. We're planning on evaluating whether it's safe to enable by default under certain conditions, but haven't gotten that far yet. It may end up remaining opt-in via this env var though, in which case we would document it.

  1. Is this new error (related to the duplex option) a known issue when using VERCEL_REMIX_NATIVE_FETCH=1 in Vercel Serverless Functions?

I haven't seen that one personally. If you could provide a repro then we could take a look.

  1. Are there any additional configuration steps required when enabling VERCEL_REMIX_NATIVE_FETCH to ensure compatibility with Vercel Serverless Functions?

Nope. Set the env var and then native fetch will be used in your serverless functions.

@coji
Copy link
Author

coji commented Jul 14, 2024

@TooTallNate

Thank you for the detailed explanation. This information is very helpful.

VERCEL_REMIX_NATIVE_FETCH env var is indeed for the purpose of setting nativeFetch: true. Feel free to use it today.

Nope. Set the env var and then native fetch will be used in your serverless functions.

I understand points 1, 2, and 4 clearly now. The clarification on the use and implementation of VERCEL_REMIX_NATIVE_FETCH is much appreciated.

Regarding point 3:

I haven't seen that one personally. If you could provide a repro then we could take a look.

It seems the issue occurs when attempting to fetch with streaming from OpenAI on the server side. I'll create a reproduction and open a separate issue to address this specific problem in more detail.

Thank you again for your support and attention to this matter.

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

No branches or pull requests

2 participants