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

Not work with Node.js v21 when using https in dev server #15495

Closed
7 tasks done
hronro opened this issue Jan 3, 2024 · 7 comments
Closed
7 tasks done

Not work with Node.js v21 when using https in dev server #15495

hronro opened this issue Jan 3, 2024 · 7 comments

Comments

@hronro
Copy link
Contributor

hronro commented Jan 3, 2024

Describe the bug

The dev server always responds "Invalid request body"

The original issue can be found at sveltejs/kit#11213.

I believe the problem lies in the incompatibility between the vite dev server and HTTP/2, which is the default protocol when using HTTPS.

While many people only use HTTP on their dev servers to avoid this issue, we have specific cookie strategies that require both our dev server and production server to use HTTPS.

UPDATE: I recently upgraded to SvelteKit 2 (which is using Vite 5) and Node.js v21.5.0, the issue persists with a different error message:

TypeError: Headers.append: ":method" is an invalid header name.
    at webidl.errors.exception (node:internal/deps/undici/undici:1636:14)
    at webidl.errors.invalidArgument (node:internal/deps/undici/undici:1647:28)
    at appendHeader (node:internal/deps/undici/undici:2053:29)
    at fill (node:internal/deps/undici/undici:2039:11)
    at new Request (node:internal/deps/undici/undici:6151:13)

Reproduction

https://github.com/hronro/vite-https-issue

Steps to reproduce

  • Run pnpm i
  • Run pnpm dev
  • Open the page in browser

System Info

System:
    OS: Linux 6.6 Arch Linux
    CPU: (14) x64 AMD Ryzen 7 5800X 8-Core Processor
    Memory: 26.50 GB / 27.41 GB
    Container: Yes
    Shell: 3.7.0 - /usr/bin/fish
  Binaries:
    Node: 21.5.0 - /usr/bin/node
    Yarn: 1.22.21 - /usr/bin/yarn
    npm: 10.2.5 - /usr/bin/npm
    pnpm: 8.13.1 - /usr/bin/pnpm
  npmPackages:
    vite: ^5.0.10 => 5.0.10

Used Package Manager

pnpm

Logs

No response

Validations

@XiSenao
Copy link
Collaborator

XiSenao commented Jan 4, 2024

I think this is a way to be compatible with Request.
sveltejs/kit#3572

@hronro
Copy link
Contributor Author

hronro commented Jan 5, 2024

@XiSenao Could you please explain more details about this? The PR you linked was merged 2 years ago.

@XiSenao
Copy link
Collaborator

XiSenao commented Jan 5, 2024

I think it may be related to the changes in this logic(node 21.4.0).

function isValidHTTPToken (characters) {
  if (characters.length === 0) {
    return false
  }
  for (let i = 0; i < characters.length; ++i) {
    if (!isTokenCharCode(characters.charCodeAt(i))) {
      return false
    }
  }
  return true
}

https://github.com/nodejs/node/blob/68c8472ed9edbf1d3fbb1e46cb65b678ee52312c/deps/undici/src/lib/fetch/util.js#L140-L150
https://datatracker.ietf.org/doc/html/rfc7230#page-27

It seems that Request is not compatible with some features of H2.
Configuring proxy for Vite can downgrade to HTTP/1.1 and also avoid the occurrence of problems. ref 02cc24f
I don't know if this issue is important enough to require Vite to be compatible with it. @hronro

@bluwy
Copy link
Member

bluwy commented Jan 8, 2024

@XiSenao do you know where we could fix to fix the issue? I'm slightly inclined to close this as there had always been issues with odd-numbered Node.js versions and often it's a Node.js bug instead.

@XiSenao
Copy link
Collaborator

XiSenao commented Jan 10, 2024

I missed the notification for this issue😂. Yes, I think Node is still in an unstable stage, and I may not be inclined to be compatible with unstable versions of Node. @bluwy

@hi-ogawa
Copy link
Collaborator

hi-ogawa commented Jan 11, 2024

FYI, Remix also had a similar story as sveltekit when adapting Vite's http2:

Initially I suggested to strip out pseudo headers (:method, etc...) when converting Node's IncomingMessage class to Web Request class:

which I thought is necessary since Remix's Request/Response related polyfill (https://github.com/remix-run/web-std-io) didn't accept such headers at that time (in sveltekit case, I guess it was node-fetch polyfill).

Since Remix's polyfill is opt-in, when disabling such polyfill, it's actually working since Node 18/20's global Request (which is essentially undici's Request) was accepting pseudo headers. So, eventually Remix team decided to follow that behavior in their polyfill as well:

Apparently, Undici "fixed" this behavior in response to this issue and Node 21 is currently rejecting http2 pseudo header:

Regardless of whether this strict behavior would land in stable node or not, personally I feel this is a framework concern than Vite itself since framework is the one converting from Node-style IncomingMessage class to Web/fetch-style Request class (also deciding whether polyfill Request or not).

Btw, I made a little script to test node's http2 here https://github.com/hi-ogawa/repro-http2-pseudo-headers/blob/main/repro.mjs

@sapphi-red
Copy link
Member

Yeah, the error was happening at this line and I also think this needs to be fixed on SvelteKit side.
https://github.com/sveltejs/kit/blob/b884c943440b9871589a2e3f680adb76e54db488/packages/kit/src/exports/node/index.js#L109-L118

Although, I think we should fix the types of connect on our side, too.

// @ts-expect-error TODO: is this correct?
app,

@github-actions github-actions bot locked and limited conversation to collaborators Nov 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants