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

fix(http): adjust fetch params to conform to the specification #57

Merged
merged 3 commits into from
Feb 17, 2025

Conversation

Convly
Copy link
Member

@Convly Convly commented Feb 14, 2025

What does it do?


TL;DR: Single-line fix: here


In the HTTP client, call fetch with

fetch(request);

instead of

fetch(url, request);

where

  • url is an instance of URL
  • request is an instance of Request

Note: Also adds a next-server-components demo project to ease later development or debugging work related to next projects.

Why is it needed?

fetch is defined as

fetch(input: RequestInfo | URL, init?: RequestInit): Promise<Response>

Where RequestInfo is Request | string, and RequestInit an object that contains information about a request.

Previously, we were sending both a URL as input and a Request instance as init, this was working fine because libraries and runtime envs were only getting properties from the init and were not trying to modify or clone it; and because the Request instances are exposing mostly the same API as the regular init object.

However, as revealed thanks to #54, Next intercepts HTTP requests by patching the globalThis.fetch method and injecting custom logic for caching and other proprietary features. While doing so, it expects both parameters to be of the correct type (but won't throw an error if that's not the case).

This led to a situation where, while trying to use the init object, Next would get confused and ignore some parts (like the custom headers).

By always using a Request as the first and only parameter (which also contains the correct URL) of the fetch call, we're able to mitigate the faulty behavior while keeping the same external interface for users.

How to test it?

Setup the repo

  • pnpm install
  • pnpm build

Using the Next demo

  • pnpm demo:setup
  • pnpm demo:start
  • cd demo/next-server-components
  • pnpm dev

Using the unit tests

Or alternatively, runs the unit tests with pnpm test:cov

Related issue(s)/PR(s)

fix #54

Copy link
Contributor

@innerdvations innerdvations left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well that's frustrating 😆

Glad you figured it out! Do you want to add the next demo to the setup process?

    "demo:build": "pnpm run build && pnpm -C demo/node-typescript build && pnpm -C demo/next-server-components build",
    "demo:env": "test -f demo/.strapi-app/.env || cp demo/.strapi-app/.env.example demo/.strapi-app/.env",
    "demo:install": "pnpm install && pnpm -C demo/.strapi-app install && pnpm -C demo/node-typescript install && pnpm -C demo/node-javascript install && pnpm -C demo/next-server-components install",
    "demo:seed": "pnpm -C demo/.strapi-app seed:example",
    "demo:seed:clean": "pnpm exec rimraf demo/.strapi-app/.tmp/data.db && pnpm run demo:seed",
    "demo:setup": "pnpm demo:install && pnpm run demo:env && pnpm run demo:build && pnpm run demo:seed:clean",
    "demo:start": "pnpm -C demo/.strapi-app develop"

After this we should probably add an actual demo setup script to keep it more manageable

Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like it

@Convly
Copy link
Member Author

Convly commented Feb 17, 2025

Well that's frustrating 😆

Glad you figured it out! Do you want to add the next demo to the setup process?

    "demo:build": "pnpm run build && pnpm -C demo/node-typescript build && pnpm -C demo/next-server-components build",
    "demo:env": "test -f demo/.strapi-app/.env || cp demo/.strapi-app/.env.example demo/.strapi-app/.env",
    "demo:install": "pnpm install && pnpm -C demo/.strapi-app install && pnpm -C demo/node-typescript install && pnpm -C demo/node-javascript install && pnpm -C demo/next-server-components install",
    "demo:seed": "pnpm -C demo/.strapi-app seed:example",
    "demo:seed:clean": "pnpm exec rimraf demo/.strapi-app/.tmp/data.db && pnpm run demo:seed",
    "demo:setup": "pnpm demo:install && pnpm run demo:env && pnpm run demo:build && pnpm run demo:seed:clean",
    "demo:start": "pnpm -C demo/.strapi-app develop"

After this we should probably add an actual demo setup script to keep it more manageable

I'll handle adding the next demo & scripting part in a follow up PR, but nice catch !

@Convly Convly merged commit ff16d51 into main Feb 17, 2025
7 checks passed
@Convly Convly deleted the fix/fetch-params branch February 17, 2025 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: fix Bug fix source: client-http Source is the client HTTP client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug]: Next.js Server Components - Missing Headers in Outbound Requests
3 participants