feat(remix-node)!: don't export fetch API

[MichaelDeBoey]

Follow-up of #7271, #7205, #7294 & @jacob-ebey's #7230

People should call installGlobals instead

[changeset-bot]

🦋 Changeset detected

Latest commit: c3a37eb

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

This PR includes changesets to release 16 packages

Name	Type
@remix-run/node	Major
@remix-run/architect	Major
@remix-run/express	Major
@remix-run/serve	Major
@remix-run/testing	Major
@remix-run/dev	Major
create-remix	Major
remix	Major
@remix-run/cloudflare	Major
@remix-run/cloudflare-pages	Major
@remix-run/cloudflare-workers	Major
@remix-run/css-bundle	Major
@remix-run/deno	Major
@remix-run/eslint-config	Major
@remix-run/react	Major
@remix-run/server-runtime	Major

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]

I agree people should be calling installGlobals, but that doesn't mean we can't keep re-exporting these. Until the built-in fetch is fully stable and we use it directly we should still have these available via @remix-run/node.

[MichaelDeBoey]

@brophdawg11 I would disagree with this
We're working towards a major release, so we can just tell people that they should call installGlobals instead and remove them from our exports already

That's already a smaller public API we have to maintain

[brophdawg11]

IMO, this is breaking change that serves no functional purpose.

What is the downside of continuing to allow folks to import { fetch } from "@remix-run/node"? Even when we can use native fetch - that re-export still doesn't need to go away. Apps just don't call installGlobals and don't import from our polyfill. It even makes it very explicit where it's coming from - which folks may prefer.

As much as I think we all want to - I don't think we are anywhere close to being able to use native fetch. Node 18 is in Maintenance LTS until 2025 so Remix will continue to support node 18 via polyfills for a very long time. Even if they they stabilize fetch and backport it into a new version of node 18 - we still can't drop out polyfills because there will be Remix apps in the wild on versions of node 18 that existed prior to stabilization.

So in my view - we need to allow folks to opt-into native fetch (which we do if they just stop calling installGlobals), but there's no need at the moment to try to shed any of our fetch polyfill surface area.

[MichaelDeBoey]

Well that's the whole thing: once we want to drop, we can just make installGlobals a no-op & remove all the code from our codebase without it being a breaking change

If we keep exporting these functions, we'll have to keep using them

Apart from that: as part of V2 migration we're already telling people we won't call installGlobals ourselves anymore and they should do it in their own code
https://remix.run/docs/en/main/pages/v2#installglobals

[brophdawg11]

I think we want to keep using them, no? If we were to move to native fetch in v2, my approach would be to console.warn a deprecation warning from inside installGlobals but keep it with the same behavior. Then we'd remove the function entirely in v3.

That way folks still have the choice of using the polyfills in v2 until they are comfortable moving to the native implementation. We'd be deprecating the polyfill just like we deprecate any other feature - and give folks the same road of iterative adoption we aim for elsewhere.

[MichaelDeBoey]

That's indeed also a possibility if you want to do so
Not sure how that would look like with @remix-run/serve then as that one automatically calls installGlobals for people using it 🤔

But still: imo we can still do this deprecation warning even if we don't have this functionality as the public API of the Node runtime

The long term goal is to not use any adapter and/or runtime at all and we always do as much as possible to keep the public API of each adapter and runtime as much as possible the same
Removing these polyfills from the public Node Runtime API takes us again 1 step closer to that magical & perfect world and v2 is a perfect moment to do so imo

[brophdawg11]

I talked to @mjackson and he's OK with removing these exports and preventing folks from importing the polyfills. This means they'll only be able to use the global instances/types. This means polyfilled folks will be getting types from TS lib.dom but instances from our polyfills. This opens up a surface area for incorrect types in cases where our polyfills are not fully spec-compliant - and the onus will have tov be on us to get them spec-compliant.

[brophdawg11]

Let's get this rebased onto release-next and get it merged!

[brophdawg11]

It looks like we've got a type issue coming from the Readable here:

packages/remix-node/fetch.ts(59,25): error TS2345: Argument of type 'NodeRequestInit' is not assignable to parameter of type '(RequestInit & RequestExtraOptions) | undefined'.
  Type 'NodeRequestInit' is not assignable to type 'RequestInit & RequestExtraOptions'.
    Type 'NodeRequestInit' is not assignable to type 'RequestInit'.
      Types of property 'body' are incompatible.
        Type 'BodyInit | Readable | null | undefined' is not assignable to type 'BodyInit | null | undefined'.
          Type 'Readable' is not assignable to type 'BodyInit | null | undefined'.

That came from #2736 which states "allow for ReadableStream's to be used as a body in node" so it sounds like it's relevant at first glance.

Internally, the WebRequest allows ReadableStream bodies via:

type BodyInit = ReadableStream | XMLHttpRequestBodyInit;

But it feels like we need/want node's stream.Readable to be able to be used too?

[brophdawg11]

Michael and I checked out this TS error above and he think's we can actually just get rid of the wrapper export const fetch since all its doing is compress:false and he thinks that's just leftover from when we were using node-fetch. Need to dig in to validate that assumption and see if that has consequences.

It seems right though since compress isn't even an option for fetch's second param: https://developer.mozilla.org/en-US/docs/Web/API/fetch. The fact that our typings say it is tells me we might have a bug in web-std-io we need to fix

[brophdawg11]

f79e6a5 now removes the fetch wrapper and NodeRequest/NodeResponse types and such leftover from when we used node-fetch in favor of polyfilling directly from @remix-run/web-fetch

[github-actions]

🤖 Hello there,

We just published version 2.0.0-pre.8 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 v0.0.0-nightly-1a57073-20230907 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 2.0.0-pre.9 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 v0.0.0-nightly-1fac238-20230908 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 2.0.0-pre.10 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 v0.0.0-nightly-3646f91-20230914 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!

[cliffordfajardo]

I know this issue is closed, but leaving as a breadcrumb link in case other folks upgrading from Remix V1 to V2 run into fetch not being exported from @remix-rune/node anymore

Follow up: #7567

import { fetch } from "@remix-run/node"; // ❌ Module '"@remix-run/node"' has no exported member 'fetch'.ts(2305)