URL is missing in typescript definitions for RequestInfo

[akornatskyy]

node-fetch: 3.0.0

Note, jsdoc for fetch lists URL as supported input.

[ChromeQ]

Just to give a bit more info here. node-fetch v2.6.1 allowed this:

const mySpecialUrl = new URL('http://google.com');   
return fetch(mySpecialUrl);

Now it gives a TS error since fetch is defined as

export default function fetch(url: RequestInfo, init?: RequestInit): Promise<Response>;

where RequestInfo is export type RequestInfo = string | Request;

In definitely-typed the RequestInfo is export type RequestInfo = string | URLLike | Request;

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/03da7d6cdb56f4ba68a8ed34f3f12b94632f2cf1/types/node-fetch/index.d.ts#L212

I feel like I should be able to pass a new URL('http://google.com') so this is a regression imo

[jimmywarting]

Actually it's suppose to only accept a string and not any URL at all.
the spec has a step to convert an argument to a UVString or something like that.
So you could technically do: { toString() { return url } } or use symbol.toLiteral

but i stand by that TypeScript isn't a silverbullet. it's bad at handling a loosed typing language, there are many things i hate about TS, like not being able to do: new Date()+1000 and more... it's the same thing with Blob, TextDecoder & URL you can pass in any argument you like and it would be converted into a string. you can even do: new URL(location) in the browser
new Blob([false]) is also acceptable but TS don't like you to do that

[ChromeQ]

@jimmywarting from a quick test in Chrome browser (using native browser fetch) I see the url can be passed as a new URL()

I believe node-fetch should act the same as browser fetch API as it is porting the functionality to node env.
Plus, from a personal point of view, I find the URL API much nicer to use when working with searchParams rather than having to make sure my url string is formatted correctly with all the ?, & and =, not to mention arrays and escaping characters or encoding. TS just treats a string as a sting no matter what I type, but using the URL API provides a bit more structure and support to the endpoint string imo
https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams

[jimmywarting]

We are already acting as the browser, the first thing we do is taking the argument and passing it into the URL constructor to parse it and validate it. we are not restricting the input to only a string (all doe the type definition don't currently allow it...)

[ChromeQ]

We are already acting as the browser

I disagree, the browser will let me pass a URL instance, node-fetch will not as it fails the TS definition. node-fetch has the signature:

node-fetch/@types/index.d.ts

Line 179 in 2603c67

export default function fetch(url: RequestInfo, init?: RequestInit): Promise<Response>;

where RequestInfo is:

node-fetch/@types/index.d.ts

Line 118 in 2603c67

export type RequestInfo = string | Request;

Which means I cannot pass const url = new URL('https://catfact.ninja/fact'); to node-fetch, but I can in browser fetch (as per my chrome screenie)

the first thing we do is taking the argument and passing it into the URL constructor

Maybe this should be the case if the argument is typeof === 'string', but the new types included with v3.0 do not allow passing URL instances, which the browser does

we are not restricting the input to only a string

Yes you are, well, string and Request, DefinitelyTyped had a type of: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/39c90291062b269d2c4f19ca7d65369c210696aa/types/node-fetch/index.d.ts#L212

Even though the URLLike was an overly simplified interface:
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/39c90291062b269d2c4f19ca7d65369c210696aa/types/node-fetch/index.d.ts#L198

[akornatskyy]

also consider a use case when you construct an URL before passing it to fetch, e.g.:

const url = new URL(
   `path/with/${variable}/goes/${here}`,
   this.baseUrl,
);
const response = await this.fetch(url);
...

Using url.toString() would weird, isn't?

[ChromeQ]

Using url.toString() would weird, isn't?

This is my (hopefully 🤞) short term solution, I would like to revert this commit once node-fetch updates the TS signature.
I don't think it should require any code changes, as according to @jimmywarting

the first thing we do is taking the argument and passing it into the URL constructor

And the URL API is ok with passing a URL instance to the constructor

[akornatskyy]

right, that is ts thing only.

[jimmywarting]

that is ts thing only.

Correct...

Looking at lib.dom.d.ts (windows implementation of fetch) then they have type RequestInfo = Request | string;
they don't have any or URL

But I agree that it is useful do do fetch(new URL(...)), fetch(location),fetch(document.querySelector('a')) etc

in fact looking at the spec of the fetch api it also says:
typedef (Request or USVString) RequestInfo;

nowhere dose it say it supports a URL as an input, the magic lies in converting the argument to a USVString if the input isn't a Request object

---

It would be sweet if TypeScript somehow could different a pure string and a USVString that gets converted into a string by diffrent means like looking at .toString() and symbol.toLiteral and so on...

[Maxim-Mazurok]

Continuing discussion from #1695:
I see, thank you for explaining, it is an interesting subject indeed.
I can see Jimmy's point about automatic conversions. However I think that this is one thing that TS encourages us to avoid, because fetch({}) will fetch ./[object%20Object] which isn't quite obvious and very likely to be a mistake. On other hand, when we're passing URL to fetch() it's extremely likely that our code is doing what we intended to do, probably even more likely than in the case of string because it can be any string. Probably the only case when URL shouldn't be passed into fetch() is when it has file:/// schema, not sure. number type might make sense in some scenarios, however, this will result in the possibility of some non-obvious conversions, such as fetch(Number.POSITIVE_INFINITY) will fetch Infinity, so probably best to force number conversions to be explicit.

I also explored an option to make it accept anything that implements toString method:

diff --git a/node_modules/node-fetch/@types/index.d.ts b/node_modules/node-fetch/@types/index.d.ts
index f68dd28..0541422 100644
--- a/node_modules/node-fetch/@types/index.d.ts
+++ b/node_modules/node-fetch/@types/index.d.ts
@@ -145,9 +145,9 @@ export interface Body extends Pick<BodyMixin, keyof BodyMixin> {}
 
 export type RequestRedirect = 'error' | 'follow' | 'manual';
 export type ReferrerPolicy = '' | 'no-referrer' | 'no-referrer-when-downgrade' | 'same-origin' | 'origin' | 'strict-origin' | 'origin-when-cross-origin' | 'strict-origin-when-cross-origin' | 'unsafe-url';
-export type RequestInfo = string | Request;
-export class Request extends BodyMixin {
-	constructor(input: RequestInfo, init?: RequestInit);
+export type RequestInfo<T extends {toString: () => string}> = string | T | Request<T>;
+export class Request<T extends {toString: () => string}> extends BodyMixin {
+	constructor(input: RequestInfo<T>, init?: RequestInit);
 
 	/**
 	 * Returns a Headers object consisting of the headers associated with request. Note that headers added in the network layer by the user agent will not be accounted for in this object, e.g., the "Host" header.
@@ -177,7 +177,7 @@ export class Request extends BodyMixin {
 	 * A referrer policy to set request’s referrerPolicy.
 	 */
 	readonly referrerPolicy: ReferrerPolicy;
-	clone(): Request;
+	clone(): Request<T>;
 }
 
 type ResponseType = 'basic' | 'cors' | 'default' | 'error' | 'opaque' | 'opaqueredirect';
@@ -216,4 +216,4 @@ export class AbortError extends Error {
 }
 
 export function isRedirect(code: number): boolean;
-export default function fetch(url: RequestInfo, init?: RequestInit): Promise<Response>;
+export default function fetch<T extends {toString: () => string}>(url: RequestInfo<T>, init?: RequestInit): Promise<Response>;

However, this will make fetch accept numbers and empty objects, which can lead to or indicate mistakes in code.

So just adding URL seems to me like a better option, covering majority of expected uses (string and URL) and not allowing unexpected conversions:

diff --git a/node_modules/node-fetch/@types/index.d.ts b/node_modules/node-fetch/@types/index.d.ts
index f68dd28..286fee5 100644
--- a/node_modules/node-fetch/@types/index.d.ts
+++ b/node_modules/node-fetch/@types/index.d.ts
@@ -145,7 +145,7 @@ export interface Body extends Pick<BodyMixin, keyof BodyMixin> {}
 
 export type RequestRedirect = 'error' | 'follow' | 'manual';
 export type ReferrerPolicy = '' | 'no-referrer' | 'no-referrer-when-downgrade' | 'same-origin' | 'origin' | 'strict-origin' | 'origin-when-cross-origin' | 'strict-origin-when-cross-origin' | 'unsafe-url';
-export type RequestInfo = string | Request;
+export type RequestInfo = string | URL | Request;
 export class Request extends BodyMixin {
 	constructor(input: RequestInfo, init?: RequestInit);

I have created PR with a proposed change in case if you find the arguments above reasonable: #1696

[LinusU]

So just adding URL seems to me like a better option, covering majority of expected uses (string and URL) and not allowing unexpected conversions

Sounds good to me, it's what the official TypeScript typings does as well 👍

[github-actions]

🎉 This issue has been resolved in version 3.3.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀