refactor(dev/vite): use @remix-run/node-fetch-server

[MichaelDeBoey]

@acusti mentioned in #12774 (comment) that @mjackson/node-fetch-server @remix-run/node-fetch-server could be used as a drop-in replacement for the current node-adapter.ts file

This started my journey and I came to the following history of the file:

1. @pcattori started it all in Cloudflare support for Vite remix#8531
2. @hi-ogawa added support for a custom basename
   Since this is not really supported by @mjackson/node-fetch-server @remix-run/node-fetch-server, I kept the small wrapper.
   @mjackson if a custom basename would be supported in the lib, then I probably looked over it, so please point me towards the correct docs
   If it's not (yet) available in the lib and you would like to somehow support it, I'm happy to help out with a PR
3. @brophdawg11's 3285264 (original PR Proxy request.signal through in vite dev remix#9976) is already implemented in https://github.com/remix-run/remix/blob/51fcb653252b48d47c32b0959d245a0400b818ac/packages/node-fetch-server/src/lib/request-listener.ts#L132-L134
4. However, @brophdawg11's d386c0d (original PR Fix adapter logic for aborting requests remix#10046) isn't implemented (yet).
   @mjackson I created fix(node-fetch-server): fix logic for aborting requests remix#10726 to do the same in @mjackson/node-fetch-server @remix-run/node-fetch-server
5. @timdorr's fix(dev/vite): Strip HTTP/2 pseudo headers from dev server requests  #12830 seems to be already implemented by skipping all headers that start with : in createHeaders
   https://github.com/remix-run/remix/blob/51fcb653252b48d47c32b0959d245a0400b818ac/packages/node-fetch-server/src/lib/request-listener.ts#L180
6. @jacob-briscoe's fix(dev): conditionally set status message for HTTP/2 compatibility #13460 doesn't seem to be implemented in sendResponse (yet).
   @mjackson if it would be supported in the lib, then I probably looked over it, so please point me towards the correct docs
   If it's not (yet) available in the lib and you would like to somehow support it, I'm happy to help out with a PR
7. @TrySound's fix(dev/vite): properly handle https protocol in dev mode #13746 (resubmission of fix(dev): detect https protocol remix#10199) is already handled in createRequest
   https://github.com/remix-run/remix/blob/51fcb653252b48d47c32b0959d245a0400b818ac/packages/node-fetch-server/src/lib/request-listener.ts#L139-L140
   @mjackson I just created refactor(node-fetch-server): add type-safety to socket check in createRequest remix#10695 to make it a bit more type-safe

So it seems like once remix-run/remix#10726 is merged (and released) and 2️⃣ & 6️⃣ have more clarity, we can indeed just replace the current node-adapter.ts file with @mjackson/node-fetch-server @remix-run/node-fetch-server.

Added benefit here is that we don't need to rely on set-cookie-parser anymore and we now rely on a lib that's going to be part of Remix v3 anyways

---

I'm sure people from the wider @e18e ecosystem cleanup (like @43081j, @benmccann & @outslept) will be very happy to see these kind of changes as well

[changeset-bot]

🦋 Changeset detected

Latest commit: 54dd667

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 11 packages

Name	Type
@react-router/dev	Patch
@react-router/fs-routes	Patch
@react-router/remix-routes-option-adapter	Patch
create-react-router	Patch
react-router	Patch
react-router-dom	Patch
@react-router/architect	Patch
@react-router/cloudflare	Patch
@react-router/express	Patch
@react-router/node	Patch
@react-router/serve	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[brophdawg11]

Thanks for the detailed overview! I'm in favor of simplifying this down if @pcattori and/or @markdalgleish are 👍

• I don't expect a concept of a basename to make it's way into remix-the-web so let's keep the wrapper here for (2) above
• Thank you for opening the PR for (4) - I'll check in with @mjackson on that
• Let's see if we can repro the original issue for (6) over in remix-the-web and if so, let's open a PR for that and I'll run that by @mjackson too

[MichaelDeBoey]

@brophdawg11 Now that remix-run/remix#10726 is merged and released, the only thing that we still need is 6️⃣ I guess?
Any more insights on that already?

[brophdawg11]

It looks like (6) is not an issue in node-fetch-server because it's using res.writeHead and not assigning res.statusMessage directly. So I think we can safely ignore (6).

Does that mean this would be ready to merge then?

Note: node-fetch-server is actually just ignoring statusText entirely which is likely a bug for HTTP/1 usages so I'll file an issue with a repro for that separately.

[MichaelDeBoey]

@brophdawg11 If we can ignore 6️⃣, this PR indeed is ready to be merged I think

[brophdawg11]

I guess technically we'd lose support for statusText in dev for HTTP/1 if we merged this as-is, so we may want to wait for remix-run/remix#10745 to land.

[brophdawg11]

ok, that's merged so I think this should be good. I'll try to take a closer glance at the code and do some testing locally and get it merged in the next couple days. Thanks again for the detailed research into this one!

[MichaelDeBoey]

@brophdawg11 remix-run/remix#10745 isn't released (yet) though, so I don't know if you want to wait for the release or not?

[brophdawg11]

oh good catch - yeah I'll wait on that

[brophdawg11]

ok, node-fetch-server 0.9.0 is released, so I updated to that and resolved conflicts. Can get this merged once CI passes.

[github-actions]

🤖 Hello there,

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

Thanks!

[github-actions]

🤖 Hello there,

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

Thanks!