Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing node types #3237

Merged
merged 2 commits into from Oct 1, 2020
Merged

Fixing node types #3237

merged 2 commits into from Oct 1, 2020

Conversation

@remcohaszing
Copy link
Contributor

@remcohaszing remcohaszing commented Aug 30, 2020

The ProgressEvent type comes from the DOM lib. This is typically unwanted when using axios in a NodeJS environment.

Fixes #3219

The `ProgressEvent` type comes from the `DOM` lib. This is typically unwanted
when using axios in a NodeJS environment.
@chinesedfan
Copy link
Collaborator

@chinesedfan chinesedfan commented Sep 12, 2020

@jasonsaayman Among related PRs, I prefer this one, which simply reverts the type to any. And users can assert the type by themselves.

  • #3221, imports the DOM lib, which is not suitable for Node.js environment.
  • #3228, provides ProgressEvent definition. But it lost the information that ProgressEvent extends from Event.

Similar debates are for method/headers(#2191/#2647). axios tries to provide more detailed enums, then it will prevent users to use other values. Unless all possible values are covered. But those values can be customized.

@Guillaume-Mayer
Copy link

@Guillaume-Mayer Guillaume-Mayer commented Sep 14, 2020

I think @chinesedfan is right, simpler is better sometimes.
Out of curiosity, what is the matter with importing the DOM lib in a node environment? Since it's just typing.

@remcohaszing
Copy link
Contributor Author

@remcohaszing remcohaszing commented Sep 14, 2020

@Guillaume-Mayer

Out of curiosity, what is the matter with importing the DOM lib in a node environment? Since it's just typing.

Although these types currently don’t conflict, they might in the future. For a practical example, the types of dom and webworker do conflict, so they can’t be combined within the same project.

TypeScript should help by providing type information. If you’re working on a TypeScript project and somehow one types document.createElement('') in a file, one would expect TypeScript to fail over this.

Also a project that claims to work in a Node environment, shouldn’ require the user to specify they’re using a DOM environment, which is the current situation with Axios.

@jasonsaayman
Copy link
Collaborator

@jasonsaayman jasonsaayman commented Sep 21, 2020

@chinesedfan I like this solution most too, do you think we should add this one into 0.20.1? I am happy to merge it and close the other PR's

@chinesedfan
Copy link
Collaborator

@chinesedfan chinesedfan commented Sep 22, 2020

@jasonsaayman Sure. Friendly remind that breaking changes should be released in 0.21, instead of 0.20.1. To make things simple, we can release this PR in 0.21 together, though it looks like not breaking.

@remcohaszing
Copy link
Contributor Author

@remcohaszing remcohaszing commented Sep 22, 2020

This is non breaking.

Copy link
Collaborator

@jasonsaayman jasonsaayman left a comment

👍

@jasonsaayman jasonsaayman merged commit b7e954e into axios:master Oct 1, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dfee
Copy link

@dfee dfee commented Oct 11, 2020

While we wait for this fix to get published, create a typings.d.ts with the following:

/**
 * Fix for axios while we wait for ^0.2.1 to be published
 * merge: https://github.com/axios/axios/commit/b7e954eba3911874575ed241ec2ec38ff8af21bb
 * issue: https://github.com/axios/axios/issues/3219
 */
interface ProgressEvent {}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants