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

refactor: Replace url.parse with new URL() #701

Merged
merged 27 commits into from Mar 13, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
3b452b8
chore: replace `url.parse` with `new URL()`
xxczaki Dec 2, 2019
dd148fc
Merge branch '3.x' into whatwg-url
xxczaki Jan 7, 2020
4298eab
lint
xxczaki Jan 7, 2020
f414fb0
handle relative URLs
xxczaki Jan 7, 2020
eb9bed4
Change error message
xxczaki Feb 19, 2020
4a795e7
detect whether the url is absolute or not
xxczaki Feb 19, 2020
7c92e6e
update tests
xxczaki Mar 7, 2020
0a891fb
drop relative url support
xxczaki Mar 7, 2020
1e747be
lint
xxczaki Mar 7, 2020
dec4065
fix tests
xxczaki Mar 8, 2020
308a6dc
typo
xxczaki Mar 8, 2020
44cb625
Add information about dropped arbitrary URL support in v3.x upgrade g…
xxczaki Mar 8, 2020
a9e7bd8
set xo linting rule (node/no-deprecated-api) to on
xxczaki Mar 8, 2020
34b5595
remove the `utf8` dependency
xxczaki Mar 8, 2020
150a618
Merge branch '3.x' into whatwg-url
xxczaki Mar 8, 2020
35c5503
fix
xxczaki Mar 8, 2020
deeb85b
refactor: split tests into several files, create the `utils` directory
xxczaki Mar 8, 2020
886b3d5
Update package.json scripts & remove unnecessary xo linting rules
xxczaki Mar 8, 2020
147da1c
refactor: turn on some xo linting rules to improve code quality
xxczaki Mar 8, 2020
42631e2
fix tests
xxczaki Mar 8, 2020
ac60e7a
Remove invalid urls
xxczaki Mar 8, 2020
54a04b7
Merge branch '3.x' into whatwg-url
xxczaki Mar 8, 2020
3002b7f
Merge branch '3.x' into whatwg-url
xxczaki Mar 8, 2020
0f07427
fix merge conflict
xxczaki Mar 8, 2020
b102ffe
Merge branch 'whatwg-url' of https://github.com/node-fetch/node-fetch…
xxczaki Mar 8, 2020
e1940b0
update the upgrade guide
xxczaki Mar 9, 2020
05bb4c8
test if URLs are encoded as UTF-8
xxczaki Mar 12, 2020
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions docs/CHANGELOG.md
@@ -1,4 +1,3 @@

Changelog
=========

Expand All @@ -14,7 +13,7 @@ Changelog
- Enhance: data URI support.
- Enhance: drop existing blob implementation code and use fetch-blob as dependency instead.
- Enhance: modernise the code behind `FetchError` and `AbortError`.
- Enhance: replace deprecated `url.parse()` and `url.replace()` with the new WHATWG `new URL()`
- Enhance: replace deprecated `url.parse()` and `url.replace()` with the new WHATWG's `new URL()`
- Enhance: allow excluding a `user-agent` in a fetch request by setting it's header to null.
- Fix: `Response.statusText` no longer sets a default message derived from the HTTP status code.
- Fix: missing response stream error events.
Expand Down
10 changes: 8 additions & 2 deletions docs/v3-UPGRADE-GUIDE.md
Expand Up @@ -67,6 +67,10 @@ We are working towards changing body to become either null or a stream.

The default user agent has been changed from `node-fetch/1.0 (+https://github.com/node-fetch/node-fetch)` to `node-fetch (+https://github.com/node-fetch/node-fetch)`.

## Arbitrary URLs are no longer supported

Since in 3.x we are using the WHATWG's `new URL()`, arbitrary URL parsing will fail due to lack of base.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I felt this explanation can be better, something like:

Creating Request/Response objects with relative URLs are no longer supported

We introduce Node.js core's new URL() API in 3.x, it offers better UTF-8 support and is WHATWG URL compatible. The drawback is, given current limit of the API (nodejs/node#12682), it's not possible to support relative URL parsing without hacks.
Due to the lack of a browsing context on Node.js, we opted to drop support for relative URLs on Request/Response object, and it will now throw errors if you do so.
The main fetch() function will support absolute URLs and data url.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. Just updated the upgrade guide 😄


# Enhancements

## Data URI support
Expand All @@ -85,9 +89,11 @@ We now use the new Node.js [WHATWG-compliant URL API][whatwg-nodejs-url], so UTF

Since the v3.x required at least Node.js 10, we can utilise the new API.

## `AbortError` now uses a w3c defined message
## Creating Request/Response objects with relative URLs is no longer supported

To stay spec-compliant, we changed the `AbortError` message to `The operation was aborted.`.
We introduced Node.js `new URL()` API in 3.x, because it offers better UTF-8 support and is WHATWG URL compatible. The drawback is, given current limit of the API (nodejs/node#12682), it's not possible to support relative URL parsing without hacks.
Due to the lack of a browsing context in Node.js, we opted to drop support for relative URLs on Request/Response object, and it will now throw errors if you do so.
The main `fetch()` function will support absolute URLs and data url.

## Bundled TypeScript types

Expand Down
309 changes: 149 additions & 160 deletions package.json
@@ -1,161 +1,150 @@
{
"name": "node-fetch",
"version": "2.6.0",
"description": "A light-weight module that brings window.fetch to node.js",
"main": "dist/index.js",
"module": "dist/index.mjs",
"types": "types/index.d.ts",
"files": [
"src/**/*",
"dist/**/*",
"types/**/*.d.ts"
],
"engines": {
"node": ">=10.0.0"
},
"scripts": {
"build": "pika-pack --out dist/",
"prepare": "npm run build",
"test": "cross-env BABEL_ENV=test mocha --require @babel/register --throw-deprecation test/test.js",
"report": "cross-env BABEL_ENV=coverage nyc --reporter lcov --reporter text mocha -R spec test/test.js",
"coverage": "cross-env BABEL_ENV=coverage nyc --reporter json --reporter text mocha -R spec test/test.js && codecov -f coverage/coverage-final.json",
"lint": "xo"
},
"repository": {
"type": "git",
"url": "https://github.com/node-fetch/node-fetch.git"
},
"keywords": [
"fetch",
"http",
"promise"
],
"author": "David Frank",
"license": "MIT",
"bugs": {
"url": "https://github.com/node-fetch/node-fetch/issues"
},
"homepage": "https://github.com/node-fetch/node-fetch",
"funding": {
"type": "opencollective",
"url": "https://opencollective.com/node-fetch"
},
"devDependencies": {
"@babel/core": "^7.8.7",
"@babel/preset-env": "^7.8.7",
"@babel/register": "^7.8.6",
"@pika/pack": "^0.5.0",
"@pika/plugin-build-node": "^0.9.2",
"@pika/plugin-build-types": "^0.9.2",
"@pika/plugin-copy-assets": "^0.9.2",
"@pika/plugin-standard-pkg": "^0.9.2",
"abort-controller": "^3.0.0",
"abortcontroller-polyfill": "^1.4.0",
"chai": "^4.2.0",
"chai-as-promised": "^7.1.1",
"chai-iterator": "^3.0.2",
"chai-string": "^1.5.0",
"codecov": "^3.6.5",
"cross-env": "^7.0.2",
"form-data": "^3.0.0",
"mocha": "^7.1.0",
"nyc": "^15.0.0",
"parted": "^0.1.1",
"promise": "^8.1.0",
"resumer": "0.0.0",
"string-to-arraybuffer": "^1.0.2",
"xo": "^0.27.2"
},
"dependencies": {
"data-uri-to-buffer": "^3.0.0",
"fetch-blob": "^1.0.5",
"utf8": "^3.0.0"
},
"@pika/pack": {
"pipeline": [
[
"@pika/plugin-standard-pkg"
],
[
"@pika/plugin-build-node"
],
[
"@pika/plugin-build-types"
],
[
"@pika/plugin-copy-assets",
{
"files": [
"externals.d.ts"
]
}
]
]
},
"xo": {
"envs": [
"node",
"browser"
],
"rules": {
"valid-jsdoc": 0,
"no-multi-assign": 0,
"complexity": 0,
"unicorn/prefer-spread": 0,
"promise/prefer-await-to-then": 0,
"no-mixed-operators": 0,
"eqeqeq": 0,
"no-eq-null": 0,
"no-negated-condition": 0,
"prefer-named-capture-group": 0,
"unicorn/catch-error-name": 0,
"node/no-deprecated-api": 1
},
"ignores": [
"dist"
],
"overrides": [
{
"files": "test/**/*.js",
"envs": [
"node",
"mocha"
],
"rules": {
"max-nested-callbacks": 0,
"no-unused-expressions": 0,
"eslint-comments/no-unused-disable": 0,
"new-cap": 0,
"guard-for-in": 0,
"no-new": 0
}
},
{
"files": "example.js",
"rules": {
"import/no-extraneous-dependencies": 0
}
}
]
},
"babel": {
"presets": [
[
"@babel/preset-env",
{
"targets": {
"node": true
}
}
]
]
},
"nyc": {
"require": [
"@babel/register"
],
"sourceMap": false,
"instrument": false
},
"runkitExampleFilename": "example.js"
}
"name": "node-fetch",
"version": "2.6.0",
"description": "A light-weight module that brings window.fetch to node.js",
"main": "dist/index.js",
"module": "dist/index.mjs",
"types": "types/index.d.ts",
"files": [
"src/**/*",
"dist/**/*",
"types/**/*.d.ts"
],
"engines": {
"node": ">=10.0.0"
},
"scripts": {
"build": "pika-pack --out dist/",
"prepare": "npm run build",
"test": "cross-env BABEL_ENV=test mocha --require @babel/register --throw-deprecation test/*.js",
"report": "cross-env BABEL_ENV=coverage nyc --reporter lcov --reporter text mocha -R spec test/*.js",
"coverage": "cross-env BABEL_ENV=coverage nyc --reporter json --reporter text mocha -R spec test/*.js && codecov -f coverage/coverage-final.json",
"lint": "xo"
},
"repository": {
"type": "git",
"url": "https://github.com/node-fetch/node-fetch.git"
},
"keywords": [
"fetch",
"http",
"promise"
],
"author": "David Frank",
"license": "MIT",
"bugs": {
"url": "https://github.com/node-fetch/node-fetch/issues"
},
"homepage": "https://github.com/node-fetch/node-fetch",
"funding": {
"type": "opencollective",
"url": "https://opencollective.com/node-fetch"
},
"devDependencies": {
"@babel/core": "^7.8.4",
"@babel/preset-env": "^7.8.4",
"@babel/register": "^7.8.3",
"@pika/pack": "^0.5.0",
"@pika/plugin-build-node": "^0.8.1",
"@pika/plugin-build-types": "^0.9.2",
"@pika/plugin-copy-assets": "^0.8.1",
"@pika/plugin-standard-pkg": "^0.9.2",
"abort-controller": "^3.0.0",
"abortcontroller-polyfill": "^1.3.0",
"chai": "^4.2.0",
"chai-as-promised": "^7.1.1",
"chai-iterator": "^3.0.2",
"chai-string": "^1.5.0",
"codecov": "^3.6.4",
"cross-env": "^7.0.0",
"form-data": "^3.0.0",
"mocha": "^7.0.0",
"nyc": "^15.0.0",
"parted": "^0.1.1",
"promise": "^8.0.3",
"resumer": "0.0.0",
"string-to-arraybuffer": "^1.0.2",
"xo": "^0.26.1"
},
"dependencies": {
"data-uri-to-buffer": "^3.0.0",
"fetch-blob": "^1.0.5"
},
"@pika/pack": {
"pipeline": [
[
"@pika/plugin-standard-pkg"
],
[
"@pika/plugin-build-node"
],
[
"@pika/plugin-build-types"
],
[
"@pika/plugin-copy-assets",
{
"files": [
"externals.d.ts"
]
}
]
]
},
"xo": {
"envs": [
"node",
"browser"
],
"rules": {
"complexity": 0,
"promise/prefer-await-to-then": 0,
"no-mixed-operators": 0,
"no-negated-condition": 0
},
"ignores": [
"dist"
],
"overrides": [
{
"files": "test/**/*.js",
"envs": [
"node",
"mocha"
],
"rules": {
"max-nested-callbacks": 0,
"no-unused-expressions": 0,
"new-cap": 0,
"guard-for-in": 0
}
},
{
"files": "example.js",
"rules": {
"import/no-extraneous-dependencies": 0
}
}
]
},
"babel": {
"presets": [
[
"@babel/preset-env",
{
"targets": {
"node": true
}
}
]
]
},
"nyc": {
"require": [
"@babel/register"
],
"sourceMap": false,
"instrument": false
},
"runkitExampleFilename": "example.js"
}
8 changes: 4 additions & 4 deletions src/body.js
Expand Up @@ -26,7 +26,7 @@ export default function Body(body, {
size = 0,
timeout = 0
} = {}) {
if (body == null) {
if (body === null) {
// Body is undefined or null
body = null;
} else if (isURLSearchParams(body)) {
Expand Down Expand Up @@ -293,7 +293,7 @@ export function clone(instance, highWaterMark) {
*/
export function extractContentType(body) {
// Body is null or undefined
if (body == null) {
if (body === null) {
Comment on lines 295 to +296
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is no longer accurate since this now only checks for null (=== null) and doesn't include undefined (== null)

This is present in a number of places...

return null;
}

Expand Down Expand Up @@ -342,7 +342,7 @@ export function extractContentType(body) {
*/
export function getTotalBytes({body}) {
// Body is null or undefined
if (body == null) {
if (body === null) {
return 0;
}

Expand Down Expand Up @@ -373,7 +373,7 @@ export function getTotalBytes({body}) {
* @returns {void}
*/
export function writeToStream(dest, {body}) {
if (body == null) {
if (body === null) {
// Body is null
dest.end();
} else if (isBlob(body)) {
Expand Down
1 change: 1 addition & 0 deletions src/errors/fetch-error.js
Expand Up @@ -23,6 +23,7 @@ export default class FetchError extends Error {

// When err.type is `system`, err.erroredSysCall contains system error and err.code contains system error code
if (systemError) {
// eslint-disable-next-line no-multi-assign
this.code = this.errno = systemError.code;
this.erroredSysCall = systemError;
}
Expand Down