Breaking: Revamp TypeScript declarations by tinovyatkin · Pull Request #810 · node-fetch/node-fetch

tinovyatkin · 2020-05-21T05:05:52Z

The decision to bring types declaration bundled since version 3.0 is a good move, however, it also makes maintainers of the project responsible for the quality of declarations.

Unfortunately, current typing has several problems:

It's buggy:

Includes references to AbortSignal without including reference to the dom lib, so, it will not compile in a project that doesn't have it included (see the included test as an example).
Declares referrer on the body while README explicitly says we don't have that
Declares getAll on Headers while we don't have such thing in code anymore
etc.

It's overblown and irrelevant. It declares a lot of things that have nothing to do with the functionality of this library, like RequestContext or ResponseType, and exports things that we actually don't export.

So, this PR proposes completely different, lightweight, and minimal approach - it uses types, declared in DOM library included with TypeScript, and extends or shrinks them whether possible and required. The resulted declaration is smaller, more compatible, and easier to maintain and read.

As it dedicated to version 3.0 it's OK to have some breaking changes and it's includes breaking functionality for users migrating from @types/node-fetch, most notable:

response.json() method returns unknown, not any. It's a good practice and even a reference example of a use for unknown type when it was introduced:
This is useful for APIs that want to signal “this can be any value, so you must perform some type of checking before you use it”. This forces users to safely introspect returned values.
we now export from declaration only things that we actually export from the library (i.e. types now like in case if the library were written in TypeScript) - Request, Response, Headers, FetchError, AbortError and fetch (default export) itself. Nothing else (the previous version was product in itself, exporting even RequestContext enum that we don't have in code).

To guarantee our typing declaration consistency I also included small automated test for GitHub action.

PS: It doesn't include any changes to JS code, so, Travis is just flacky.

test/types/tsconfig.json

LinusU

LGTM w/ a few comments

Have you considered using tsd to test the type definitions, instead of just using tsc. It gives you expectType<T>() so that you can actually check that retured types are what you expect.

e.g.

// ok must be boolean, so do strict equality
if (getRes.ok !== true) ...

will not raise a warning if ok was typed as true | number | 'foobar', since it's still valid to compare it with true. But with tsd you would be able to do:

expectType<boolean>(getRes.ok)
if (getRes.ok !== true) ...

It also supports top-level await so you can remove async function run :)

Co-authored-by: Linus Unnebäck <linus@folkdatorn.se>

tinovyatkin · 2020-05-21T12:04:02Z

@LinusU I didn't want to introduce a new testing framework for types and resort to this kind of "functional testing" by compilation a file via the latest version of TypeScript on the GitHub Actions.
There are certain areas to improve even further here, but at the moment I feel it's a good step forward and would prefer to wait for first real-world problem feedback from users to understand how to better deal with potential problems should they arrive.

jstewmon · 2020-05-21T12:15:39Z

Referencing dom lib causes all sorts of problems for node projects. This has been an ongoing problem with superagent for a long time. Please reconsider this approach.

See: DefinitelyTyped/DefinitelyTyped#12044 (comment)

There are some very painful conflicts like the timer interfaces and globals such as URL.

I recently switched from superagent to node-fetch to avoid these problems.

tinovyatkin · 2020-05-21T21:48:54Z

@jstewmon I should agree that lack of module isolation (by default) in TypeScript may cause background injection of lib.dom.d.ts and I agree too that's it's not the desired side-effect for many pure Node projects (I'm personally would not love that) - thank you for this insight!

I will bring inside all our types (at better review it's not that much we need to bring in).
@Richienb could you do the same for fetch-blob that currently reexports Blob from lib.dom, so, I will import Blob from the fetch-blob then?

tinovyatkin · 2020-05-21T21:54:58Z

@LinusU we probably need to revert node-fetch/fetch-blob@bec5a3d
Also export = declaration requires synthetic default imports enabled, is there any particular reason why you avoiding export default Blob?

LinusU · 2020-05-22T10:31:01Z

we probably need to revert node-fetch/fetch-blob@bec5a3d

Hmm, yeah, we can probably copy the classes from dom.d.ts right off though 🤔

Also export = declaration requires synthetic default imports enabled, is there any particular reason why you avoiding export default Blob?

The types should always reflect what's in the source file, and since this is the source:

module.exports = Blob

The correct typing is export = Blob. I think that you'll have problem importing it in some setups if the types don't match the source. Here is some guidelines from DefinitelyTyped: https://github.com/DefinitelyTyped/DefinitelyTyped#a-package-uses-export--but-i-prefer-to-use-default-imports-can-i-change-export--to-export-default

Richienb · 2020-05-23T01:45:46Z

index.d.ts

-	highWaterMark?: number; // =16384 the maximum number of bytes to store in the internal buffer before ceasing to read from the underlying resource.
+import { Agent } from 'http';
+import { URL, URLSearchParams } from 'url'
+import { AbortSignal } from 'abort-controller'


abort-controller is not a dependency so it won't work out of the box. Also, node-fetch is supposed to be a lightweight implementation and adding this might be weighing it down.

@Richienb actually abort-controller is a devDependency of this project for quite a long time. What will be your proposed alternative for referring AbortSignal here - bring it from lib.dom or copy the whole definition inside (it's quite big)?

Is it possible to import from lib.dom using ES6 imports?

Nope, lib.dom is a library, not a module. And you probably should double-check the @jstewmon comment above about downside of just referring lib.dom in a triple-slash comment.

xxczaki

Just like @LinusU said - we should use tsd for testing types.

Richienb

The files in @types can probably live in the root directory.

tinovyatkin · 2020-05-23T18:16:57Z

I'm OK to move them whatever place the team will decide, but what's particularly wrong with @types folder?

xxczaki · 2020-05-23T18:21:23Z

For me, it doesn't matter where the type tests will be placed ;)

xxczaki · 2020-05-24T10:43:28Z

Are we ready to merge or not yet?

xxczaki

Actually, I think we need to document this change in the upgrade guide.

tinovyatkin · 2020-05-24T12:41:43Z

For me this is pretty much done and ready for merge. The fact that v3.0 comes bundled with types is documented on README at TypeScript section.

Richienb

The @ symbol in @types is unnecessary. I would personally call it typings but types would do.

xxczaki · 2020-05-24T12:52:36Z

Let's change the name to types, as it seems like most projects call it like so.

tinovyatkin · 2020-05-24T14:51:15Z

That's a great Sunday discussion on one symbol in the folder name at project internal structure 😄
Let's make some 🔥

@xxczaki I don't have statistics (would be nice to have) how many projects have it at types vs @types, but for me, a good vote for @types is default support in vscode-icons - most popular Visual Studio Code files icon theme (and in Top 10 of all VSCode extensions). I do believe that @types will be better user experience even after @Richienb concerns of an extra symbol:

xxczaki · 2020-05-24T14:53:33Z

@tinovyatkin I mean, if one symbol improves DX just a bit and does not really affect any users, we can go for it 😆

xxczaki

I think we can merge this unless @Richienb or the others have any additional suggestions.

In terms of the directory naming - we can always change that later ;)

NotMoni · 2020-05-24T14:57:53Z

Is also support @types

NotMoni · 2020-05-24T15:01:06Z

I have one question

In the actions file don’t we need to specify the node version

NotMoni · 2020-05-24T15:02:36Z

We should change the name CI in the actions to something more specific b/c we already have a CI yml

tinovyatkin · 2020-05-24T15:04:37Z

@NotMoni

In the actions file don’t we need to specify the node version

No, GitHub Actions have LTS version installed by default: https://help.github.com/en/actions/reference/software-installed-on-github-hosted-runners
I'm running install-node action step here only because it adds problem matcher.

We should change the name CI in the actions to something more specific b/c we already have a CI yml

No, we shouldn't unless we want more badges in README. GitHub Action will sum up action by this name into one badge status, so, if somebody will push a broken typing into master - we will see it if we keep the name as CI and will not if we will change it to something else.

NotMoni · 2020-05-24T15:05:53Z

Alright 🦄

node-fetch/node-fetch#1141 and node-fetch/node-fetch#810 should be proof these lads are high on acids

node-fetch@v3 has bundled types: node-fetch/node-fetch#810

* Update @heroicons/react * Update @types/node * Update @types/react-paginate * Update node-fetch (v2 → v3) * Remove @types/node-fetch node-fetch@v3 has bundled types: node-fetch/node-fetch#810 * Update typescript * Update npm lockfile to v3 * Update tailwindcss, postcss, autoprefixer * Update tailwind config for v3 * Change classnames to tailwind v3 equivalents * Fix post-update heroicons issues * Update react-hook-form * Update react-paginate * Update prettier * Update eslint-* minor versions * Update @types/react minor version * Update next-themes * Update node-fetch * Update @typescript-eslint/* * Update eslint* * Update .gitignore * Remove next-env.d.ts Remnant of older installation. https://nextjs.org/docs/basic-features/typescript#existing-projects * Update next (v11 → v12) * Update @types/react* * Update next (v12 → v13) * Add eslint-config-next * Update eslint config Remove redundant eslint config. Sane defaults are being applied by (eslint-config-next)[https://nextjs.org/docs/basic-features/eslint#eslint-config]. * Update next config, fix issue with images src * Remove redundant eslint plugins See 8dbd0d5 * Use next components instead of plain html * Remove @types/react-paginate Types are now bundled. * Remove node-fetch * Slightly refactor api response usage

tinovyatkin added 3 commits May 21, 2020 00:37

revamp types

02f76b8

add GitHub Action for TypeScript check

3c6ff80

explicit types point

fc5aba0

LinusU reviewed May 21, 2020

View reviewed changes

test/types/tsconfig.json Outdated Show resolved Hide resolved

LinusU reviewed May 21, 2020

View reviewed changes

test/types/tsconfig.json Outdated Show resolved Hide resolved

LinusU approved these changes May 21, 2020

View reviewed changes

xxczaki added the enhancement label May 21, 2020

This was referenced May 21, 2020

v3 Roadmap #668

Closed

Typescript: Cannot read property 'headers' of undefined. Issue with reference to "this" in Body methods #808

Closed

Update test/types/tsconfig.json

87721ed

Co-authored-by: Linus Unnebäck <linus@folkdatorn.se>

tinovyatkin added 6 commits May 21, 2020 17:59

don't use lib.dom

4cef061

Merge branch 'master' into test-types

a2f8355

fix package.json merge

ea5c9c4

fib Blob

c781c27

tets errno on FetchError

4d8244e

replace import type to lower TypeScript requirements

dbb1a18

Richienb requested changes May 23, 2020

View reviewed changes

Richienb changed the title ~~[BREAKING] Revamp TypeScript declarations~~ Breaking: Revamp TypeScript declarations May 23, 2020

Merge branch 'master' into test-types

f0331b1

xxczaki requested changes May 23, 2020

View reviewed changes

tinovyatkin added 3 commits May 23, 2020 11:39

use tsd for types testing

45951c2

save with carret

9ee1061

fix CI names

35462df

xxczaki approved these changes May 23, 2020

View reviewed changes

Richienb requested changes May 23, 2020

View reviewed changes

xxczaki and others added 4 commits May 24, 2020 12:28

Merge branch 'master' into test-types

41c0280

fix ci

30ec3a6

use a local test

b806437

ignore the @types directory

cfbfe78

xxczaki approved these changes May 24, 2020

View reviewed changes

xxczaki reviewed May 24, 2020

View reviewed changes

tinovyatkin mentioned this pull request May 24, 2020

Remove timeout option #831

Merged

4 tasks

Richienb requested changes May 24, 2020

View reviewed changes

xxczaki reviewed May 24, 2020

View reviewed changes

NotMoni approved these changes May 24, 2020

View reviewed changes

tinovyatkin merged commit 4824abe into node-fetch:master May 24, 2020

tinovyatkin deleted the test-types branch May 24, 2020 15:58

elbywan mentioned this pull request Jul 6, 2020

TypeScript build errors, fetch-related types are missing elbywan/wretch#83

Closed

3 tasks

ImRodry mentioned this pull request Sep 1, 2021

v3.0.0 await res.json() is unknown #1262

Closed

ImRodry added a commit to Hypixel-Translators/hypixel-translators-bot that referenced this pull request Sep 1, 2021

Remove node-fetch in favor of axios

964886f

node-fetch/node-fetch#1141 and node-fetch/node-fetch#810 should be proof these lads are high on acids

ImRodry added a commit to Hypixel-Translators/hypixel-translators-bot that referenced this pull request Sep 1, 2021

Remove node-fetch in favor of axios

6ce33eb

node-fetch/node-fetch#1141 and node-fetch/node-fetch#810 should be proof these lads are high on acids

modkaffes added a commit to modkaffes/angorank that referenced this pull request Nov 11, 2022

Remove @types/node-fetch

a4bca3d

node-fetch@v3 has bundled types: node-fetch/node-fetch#810

Uh oh!

Conversation

tinovyatkin commented May 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LinusU left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tinovyatkin commented May 21, 2020

Uh oh!

jstewmon commented May 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tinovyatkin commented May 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tinovyatkin commented May 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LinusU commented May 22, 2020

Uh oh!

Richienb May 23, 2020

Choose a reason for hiding this comment

Uh oh!

tinovyatkin May 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Richienb May 23, 2020

Choose a reason for hiding this comment

Uh oh!

tinovyatkin May 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xxczaki left a comment

Choose a reason for hiding this comment

Uh oh!

Richienb left a comment

Choose a reason for hiding this comment

Uh oh!

tinovyatkin commented May 23, 2020

Uh oh!

xxczaki commented May 23, 2020

Uh oh!

xxczaki commented May 24, 2020

Uh oh!

xxczaki left a comment

Choose a reason for hiding this comment

Uh oh!

tinovyatkin commented May 24, 2020

Uh oh!

Richienb left a comment

Choose a reason for hiding this comment

Uh oh!

xxczaki commented May 24, 2020

Uh oh!

tinovyatkin commented May 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xxczaki commented May 24, 2020

Uh oh!

xxczaki left a comment

Choose a reason for hiding this comment

Uh oh!

NotMoni commented May 24, 2020

Uh oh!

NotMoni commented May 24, 2020

Uh oh!

NotMoni commented May 24, 2020

Uh oh!

tinovyatkin commented May 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NotMoni commented May 24, 2020

Uh oh!

Reviewers

tinovyatkin commented May 21, 2020 •

edited

Loading

LinusU left a comment •

edited

Loading

jstewmon commented May 21, 2020 •

edited

Loading

tinovyatkin commented May 21, 2020 •

edited

Loading

tinovyatkin commented May 21, 2020 •

edited

Loading

tinovyatkin May 23, 2020 •

edited

Loading

tinovyatkin May 23, 2020 •

edited

Loading

tinovyatkin commented May 24, 2020 •

edited

Loading

tinovyatkin commented May 24, 2020 •

edited

Loading