Avoiding `instanceof Request`, etc.

[hansottowirtz]

In Node.js 18.6, this piece of code doesn't work:

import { fetch } from "undici";

fetch(new Request("https://example.com"));

Error:

TypeError: Failed to parse URL from [object Request]
    at fetch (<>/node_modules/undici/index.js:109:13) {
  [cause]: TypeError [ERR_INVALID_URL]: Invalid URL
      at new NodeError (node:internal/errors:405:5)
      at new URL (node:internal/url:743:13)
      at new Request (<>/node_modules/undici/lib/fetch/request.js:86:21)
      at fetch (<>node_modules/undici/lib/fetch/index.js:137:21)
      at fetch (<>/node_modules/undici/index.js:107:20)
      at file://<>/test.js:3:1
      at ModuleJob.run (node:internal/modules/esm/module_job:192:25)
      at async DefaultModuleLoader.import (node:internal/modules/esm/loader:246:24)
      at async loadESM (node:internal/process/esm_loader:40:7)
      at async handleMainPromise (node:internal/modules/run_main:66:12) {
    input: '[object Request]',
    code: 'ERR_INVALID_URL'
  }
}

This is because instanceof Request fails due to the global Request and the Undici Request not being the same.

https://github.com/nodejs/undici/blob/7153a1c78d51840bbe16576ce353e481c3934701/lib/fetch/request.js#L854

While many libraries accept a fetch argument (like ky), not many libraries accept another set of globals (Request, Response, Headers). Next to that, global.Request is a read-only property in Node.js. I think that instead of using instanceof Undici should be using another type of check for these. (e.g. request[Symbol.toStringTag] === 'Request').

I'd like to collect thoughts and then I'll create a PR.

This would also fix https://github.com/cloudflare/miniflare/issues/454

[KhafraDev]

Unfortunately loosening that check wouldn't solve the issue; we need to access the internal Request state (via a symbol) that will not work since the global fetch's symbols are different from undici's symbols. The only way I can think of solving this is to use Object.getOwnPropertySymbols and then find one names kState/kSignal. But then we'd also have other issues:

const request = {
  [Symbol('kState')]: 'lol',
  [Symbol('kSignal')]: null,
  [Symbol.toStringTag]: 'Request'
}

I'm not sure if this can be fixed, without causing other issues.

[ronag]

Use SymbolFor?

[KhafraDev]

Possible, but creating a dozen global symbols (this would need to be done for all fetch globals, not just Request) would cause more issues like https://github.com/nodejs/node/issues/43157. Alternatively we could move all undici symbols under a single global one?

globalThis[Symbol.for('undici.global.symbol')] = {
  state: Symbol('kState'),
  // etc
}

which still seems undesirable because it could be used to access the internal state of fetch objects

[mcollina]

I don't think we want to expose the internals as part of our API contract. We want to be able to change them and not worry if we would break this use case or not.

I think we should just document this quirk.

[KhafraDev]

We should still consider avoiding instanceof because it can be overridden with Symbol.hasInstance. Instead we should be doing an Object.hasOwn check and making sure, for example, that the object has a kState. I've known about this for a while now, it's a pretty small issue but spans essentially every spec we implement (fetch, CacheStorage, WebSocket) because of how we use it in webidl.

an example:

Object.defineProperty(Response, Symbol.hasInstance, {
  value: () => true
})

console.log([] instanceof Response) // true

[jimmywarting]

It's kind of a bummer that undici and builtin fetch don't play ball. i have for instance wanted to only import the CacheStorage and tree shake away the rest and hopped that i could use it together with builtin Request/Response, but it's a very strict brand checking