fix: improve TypeScript types

[SomaticIT]

What is the purpose of this pull request?

• Documentation update
• Bug fix
• New feature
• Other, please explain:

What changes did you make? (provide an overview)

• Export all types and interfaces
• Use class instead of interface+var where it's possible
• Use a minimal type for AbortSignal to avoid the need to import 'abort-controller' module
• Use method declaration instead of lambda declaration
• Avoid namespace creation that is not needed

Which issue (if any) does this pull request address?

• Impossible to construct RequestInit or ResponseInit object correctly:

const init: RequestInit = { /* ... */ };

• Impossible to build a TypeScript project with no abort-controller dependency
• Not compatible with v3.0.0-beta.5 types

Is there anything you'd like reviewers to know?

• check if isRedirect() is useable in this way.

[xxczaki]

Overall LGTM 👍

[xxczaki]

Update: everything works correctly.

[xxczaki]

check if isRedirect() is useable in this way.

The previous review contained some invalid code, I can confirm it works correctly 😆

[LinusU]

Since the main field will point to index.cjs, and the types field (which is the corresponding field for the typings) will point to this file, I think that the correct thing here would be to use a CommonJS export:

Suggested change

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

You can read some more info it here: https://github.com/DefinitelyTyped/DefinitelyTyped#a-package-uses-export--but-i-prefer-to-use-default-imports-can-i-change-export--to-export-default

[SomaticIT]

If you look at the built index.cjs, the resulting exports is the following:

exports.default = fetch;

So the good way to type it is to use export default.
export = fetch is equivalent to module.exports = fetch which is not the case here...

[LinusU]

ahh, cool, I was looking at the currently published package which uses module.exports = fetch, my bad!

[LinusU]

@SomaticIT I just cloned the repo and made a clean build, and the output in the bundle is module.exports = fetch, thus I think that we should type this as export = fetch

[SomaticIT]

Thanks all reviewers for your time!

[tinovyatkin]

My major concern with current typings is the export description. I understand that export default fetch matches with our code, etc.
However, it lacks tooling support (read VSCode IntelliSense) while the module is used as CommonJS (try with test/commonjs/test-artifact.js:

export = fetch while looks weird and requires synthetic default import solves this problem and so, results in better DX. Should we make that change?

[SomaticIT]

export = fetch while looks weird and requires synthetic default import solves this problem and so, results in better DX. Should we make that change?

I strongly disagree on this. A d.ts library should never require an option in tsc

[SomaticIT]

My major concern with current typings is the export description. I understand that export default fetch matches with our code, etc.
However, it lacks tooling support (read VSCode IntelliSense) while the module is used as CommonJS (try with test/commonjs/test-artifact.js:

export = fetch while looks weird and requires synthetic default import solves this problem and so, results in better DX. Should we make that change?

It works on my vs code...

[SomaticIT]

Important note:
The type of HeadersInit is incorrect in JSDoc:

export type HeadersInit = Headers | Record<string, string> | Iterable<readonly [string, string]> | Iterable<string>[];

Indeed, the evaluation function looks at length in iterable items (line 79):

if (pair.length !== 2) {
    throw new TypeError('Each header pair must be a name/value tuple');
}

It means that we have to replace Iterable<string>[] by Iterable<string[]>:

export type HeadersInit = Headers | Record<string, string> | Iterable<readonly [string, string]> | Iterable<string[]>;

It also performs better TypeScript check:

// Works with Iterable<string>[] but report a type error with Iterable<string[]>
const headers = new Headers(['name', 'value]);

[LinusU]

Just double checked how the module is exported from the built file, and it is exported with the following code:

exports = module.exports = fetch;

Thus the correct typing for this module would be:

export = fetch;

ref: #841 (comment)

---

If we type this as export default function fetch then you won't be able to correctly import this without esModuleInterop, since it will output: Actually we are doing fetch.default = fetch so it will still work...

var node_fetch_1 = require("node-fetch");
node_fetch_1["default"]('test');

---

without esModuleInterop

Our declaration	Users import	Transpiled code
export default function	import fetch from 'node-fetch	var node_fetch_1 = require("node-fetch"); node_fetch_1["default"]('test');
export default function	import fetch = require('node-fetch')	Error: This expression is not callable.
export = fetch	import fetch from 'node-fetch	Error: This module is declared with using 'export =', and can only be used with a default import when using the 'esModuleInterop' flag.
export = fetch	import fetch = require('node-fetch')	var fetch = require("node-fetch"); fetch('test');

with esModuleInterop

Our declaration	Users import	Transpiled code
export default function	import fetch from 'node-fetch	var node_fetch_1 = __importDefault(require("node-fetch")); node_fetch_1["default"]('test');
export default function	import fetch = require('node-fetch')	Error: This expression is not callable.
export = fetch	import fetch from 'node-fetch	var node_fetch_1 = __importDefault(require("node-fetch")); node_fetch_1["default"]('test');
export = fetch	import fetch = require('node-fetch')	var fetch = require("node-fetch"); fetch('test');

[tinovyatkin]

The type of HeadersInit is incorrect in JSDoc:

Please check the test/headers.js file for all possible ways to init Headers. Particularly badly named test should accept headers as an iterable of tuples where Headers are created from Set<string> and .keys iterator from Map.

[tinovyatkin]

I still disagree that we need to export types for internal data structures of this library even due to reason that somebody else is building TypeScript library on top of node-fetch and will need to expose original types:

1. It makes any changes to these internal data structures a breaking change for the whole library
2. These exports wouldn't be available if this library was written in TypeScript originally.
3. There is a built-in ways in TypeScript to extract types of parameters and return type and constructor parameters

[xxczaki]

@tinovyatkin Slightly unrelated to this PR, but what would you rename the should accept headers as an iterable of tuples test to?

[LinusU]

Argh, I have been looking way too long but cannot find it now. I know that I saw a comment from someone on the TypeScript team the other day explaining how to correctly type a library that exports module.exports = foo; module.exports.default = foo so that it can be imported with both CommonJS and Module style, without requiring esModuleInterpo in any of the cases. I really think that is the best approach here since we are really exporting both versions...

[tinovyatkin]

@tinovyatkin Slightly unrelated to this PR, but what would you rename the should accept headers as an iterable of tuples test to?

I'm not a native English speaking person, so, probably not the best source of such type of suggestions, but what this test actually tests is that Headers can be created from a linear iterator of string values if the accumulated array of strings can be split into tuples evenly. I mean, it's not iterable of tuples, it's iterable of strings.

[tinovyatkin]

@LinusU Do you mean this comment? microsoft/TypeScript#2242 (comment)

[LinusU]

Hmmm, no unfortunately not, this was a more recent comment. And it had some text that was basically "If your library is really exporting both CommonJS and a default name, you can do this in your typings:"...

[SomaticIT]

@LinusU Maybe this one:
microsoft/TypeScript#10854 (comment)

[LinusU]

@SomaticIT not that specific comment, but it seems to be the same trick 😍 awesome find!! 👏

With that I have managed to test this out locally and I think that this is the best approach:

type Fetch = (url: RequestInfo, init?: RequestInit) => Promise<Response>
declare const fetch: Fetch & { default: Fetch }
export = fetch

esModuleInterop	Users import	Transpiled code
false	import fetch from 'node-fetch'	var node_fetch_1 = require("node-fetch"); node_fetch_1["default"]('test');
false	import fetch = require('node-fetch')	var fetch = require("node-fetch"); fetch('test');
true	import fetch from 'node-fetch'	var node_fetch_1 = __importDefault(require("node-fetch")); node_fetch_1["default"]('test');
true	import fetch = require('node-fetch')	var fetch = require("node-fetch"); fetch('test');

This is amazing, I think that there is a lot of libraries that are missing this in their types, where the library is actually exporting both module.exports and module.exports.default 🙌

[SomaticIT]

Dear reviewers,

I think I found a very good solution that allows to do export = , export default and export types in the same time:

declare function fetch(url: RequestInfo, init?: RequestInit): Promise<Response>; // function is mergeable
declare class fetch {
	static default: typeof fetch; // default needs to be a value to use import fetch from...
}
declare namespace fetch { // namespace allows import * and exporting types
	// export types
}

export = fetch;

This solutions allows any ways of importing in TypeScript and creates compatible transpilation.
It also allows to have classes like Request and Response to be exported in the best way.

Eg:

import fetch, {Request} from "node-fetch";
asserts.equals(fetch.Request, Request);
fetch.isRedirect = (code: number) => true;

import * as fetch from "node-fetch";
asserts.equals(fetch.default.Request, fetch.Request);
fetch.isRedirect = (code: number) => true;

import fetch from require("node-fetch");
asserts.equals(fetch.default.Request, fetch.Request);
fetch.isRedirect = (code: number) => true;

With this model, I think that we match perfectly the index.cjs module exports.

What do you think?

[SomaticIT]

The type of HeadersInit is incorrect in JSDoc:

Please check the test/headers.js file for all possible ways to init Headers. Particularly badly named test should accept headers as an iterable of tuples where Headers are created from Set<string> and .keys iterator from Map.

You're right, I fixed it in my last commit with Iterable<Iterable<string>>, the only problem is that it allows new Headers(["test", "test"]) to pass type checks but will not pass runtime check.

But it's the real type for the implementation.

Resolved