Support interoperability with other version of itself

matthieusieben commented 7 months ago

This would solve...

undici is not compatible with other versions of itself, or Node's.

If a library uses undici's fetch() internally and the user of the library uses Node's Request, or vice versa, fetch() call will fail with an error like so:

  TypeError: Failed to parse URL from [object Request]
      at fetch (~/my-lib/node_modules/.pnpm/undici@6.11.1/node_modules/undici/index.js:109:13) {
    [cause]: TypeError: Invalid URL
        at new URL (node:internal/url:804:36)
        at new Request (~/my-lib/node_modules/.pnpm/undici@6.11.1/node_modules/undici/lib/web/fetch/request.js:88:21)
        at fetch (~/my-lib/node_modules/.pnpm/undici@6.11.1/node_modules/undici/lib/web/fetch/index.js:136:21)
        at fetch (~/my-lib/node_modules/.pnpm/undici@6.11.1/node_modules/undici/index.js:106:18)
        at fetch (~/my-lib/packages/pds/dist/auth-provider.js:42:51)
        at fetchTimeout (~/my-lib/packages/fetch/dist/fetch-wrap.js:61:28)
        at ~/my-lib/packages/fetch/dist/fetch-wrap.js:39:29
        at ~/my-lib/packages/transformer/dist/compose.js:20:47
        at async i (~/my-lib/packages/transformer/dist/compose.js:16:51)
        at async CachedGetter.get (~/my-lib/packages/caching/dist/cached-getter.js:97:20) {
      code: 'ERR_INVALID_URL',
      input: '[object Request]'
    }

This error is misleading as the input is actually a Request that gets casted to string because it comes from another version of undici:

https://github.com/nodejs/undici/blob/f51f226522ec75a0613a07a4efc8f78938030c45/lib/web/fetch/request.js#L924-L934

The implementation should look like...

When checking if an input is a request, instead of using instanceof, an thorough interface check could be performed. Every fields from the input should be checked against the spec's interface https://fetch.spec.whatwg.org/#requestinfo

webidl.converters.RequestInfo = function (V) {
  if (typeof V === 'string') {
    return webidl.converters.USVString(V)
  }

  if (V instanceof Request) {
    return webidl.converters.Request(V)
  }

  const str = webidl.converters.USVString(V)
  if (str === '[object Request]') {
    // Request from another version of undici (or node's internal undici)
    return webidl.converters.RequestInterface(V) // <== TODO implement this
  }

  return src
}

Note that every field of the input Request (including dispatcher) should be carried over to the new request.

Symbols would probably need to be using a globally namespaced form instead of locally declared symbols (Symbol.for('undici.XYZ')) for the initalization process to work here:

https://github.com/nodejs/undici/blob/f51f226522ec75a0613a07a4efc8f78938030c45/lib/web/fetch/request.js#L107-L118

I have also considered...

Additional context

I am trying to make a library to ease the use of fetch() by allowing to tranform Requests, and wrap node's fetch(). This is not possible if Request/fetch from different sources are mixed together

mcollina commented 7 months ago

cc @KhafraDev wdyt? I think this is needded.

KhafraDev commented 7 months ago

we've talked about it in the past and there is no good way to support it

mcollina commented 7 months ago

Then we should document it properly.

Uzlopak commented 7 months ago

Is it not a good way to support it, because it would be too complex or, or because it would eat up a lot of performance?

KhafraDev commented 7 months ago

Then we should document it properly.

Definitely

Is it not a good way to support it, because it would be too complex or, or because it would eat up a lot of performance?

Neither... well, complexity is also an issue, but it's just that the solution would be sloppy and probably wouldn't work on every version of node we support. For example: what if undici releases a minor version that updates internal state of Request/Response/etc. and is no longer compatible with a node version(s)? It's not impossible to work around, but there is fundamentally no good solution to the problem.

It's important to remember that while undici implements fetch, we do not control it when it's used in node. The webidl implementation we have is also meant to be spec compliant, there is no such thing as "RequestInterface" as a possible value of RequestInfo.

Why things won't work:

we can't simply convert a [whatever] to a string because then class A extends Request {} will fail the check.
instanceof doesn't work
a global Symbol changes the scope of the internal state to a public api, in the same sense of the global dispatcher one.
checking for the existence of properties/methods allows third party implementations, this is something that I plan to move away from. But it also limits us in what we define a [whatever] as, which will break everything if a new required argument is added or a new required option is added.

It's totally possible for a third party library to handle the issues themselves, access whatever internal state, and make the two compatible. Take the internal state from one, add whatever is needed (if anything), and assign it to the other. Not a good thing for undici to implement ourselves, but I can see someone else doing it.

mcollina commented 7 months ago

I think adding that script and a undici.__overrideGlobals() method would be good.

matthieusieben commented 7 months ago

I think you are right, sadly. The docs should indeed clearly indicate that undici 's exported members cannot/mustnot be used with those from another version (or Node's).

IMO, the docs should probably recommend to avoid using fetch / Request / Response & co from undici at all if the runtime supports those, and rely on something like this if an up-to-date version is required.

nodejs / undici