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.

Sign up for GitHub

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

Jump to bottom

fix: use URL constructor instead of url.parse() #33

Merged
merged 2 commits into from Feb 24, 2022
Merged

fix: use URL constructor instead of url.parse() #33

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
52be9e2
fix: use URL constructor instead of url.parse()
nlf Feb 24, 2022
57e64e8
fixup! fix: use URL constructor instead of url.parse()
nlf Feb 24, 2022
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
12 changes: 3 additions & 9 deletions lib/index.js
View file
Open in desktop
@@ -1,5 +1,5 @@
'use strict'
const Url = require('url')
const { URL } = require('url')
const http = require('http')
const https = require('https')
const zlib = require('minizlib')
Expand All @@ -15,10 +15,6 @@ const { getNodeRequestOptions } = Request
const FetchError = require('./fetch-error.js')
const AbortError = require('./abort-error.js')

// disabling no-deprecated-api because we need to refactor to switch to the URL constructor
// eslint-disable-next-line node/no-deprecated-api
const resolveUrl = Url.resolve

const fetch = (url, opts) => {
if (/^data:/.test(url)) {
const request = new Request(url, opts)
Expand Down Expand Up @@ -130,7 +126,7 @@ const fetch = (url, opts) => {

// HTTP fetch step 5.3
const locationURL = location === null ? null
: resolveUrl(request.url, location)
: (new URL(location, request.url)).toString()

// HTTP fetch step 5.5
if (request.redirect === 'error') {
Expand Down Expand Up @@ -174,9 +170,7 @@ const fetch = (url, opts) => {
}

// Update host due to redirection
// disabling no-deprecated-api pending refactor to URL constructor
// eslint-disable-next-line node/no-deprecated-api
request.headers.set('host', Url.parse(locationURL).host)
request.headers.set('host', (new URL(locationURL)).host)
wraithgar marked this conversation as resolved.
Show resolved Hide resolved

// HTTP-redirect fetch step 6 (counter increment)
// Create a new Request object.
Expand Down
34 changes: 19 additions & 15 deletions lib/request.js
View file
Open in desktop
@@ -1,5 +1,5 @@
'use strict'
const Url = require('url')
const { URL } = require('url')
const Minipass = require('minipass')
const Headers = require('./headers.js')
const { exportNodeCompatibleHeaders } = Headers
Expand All @@ -12,8 +12,6 @@ const defaultUserAgent =

const INTERNALS = Symbol('Request internals')

const { format: formatUrl } = Url

const isRequest = input =>
typeof input === 'object' && typeof input[INTERNALS] === 'object'

Expand All @@ -28,12 +26,9 @@ const isAbortSignal = signal => {

class Request extends Body {
constructor (input, init = {}) {
// we use url.parse() here because swapping for new URL() is not straight forward.
// until the necessary refactoring is done, we'll just silence the linter
// eslint-disable-next-line node/no-deprecated-api
const parsedURL = isRequest(input) ? Url.parse(input.url)
: input && input.href ? Url.parse(input.href) // eslint-disable-line node/no-deprecated-api
: Url.parse(`${input}`) // eslint-disable-line node/no-deprecated-api
const parsedURL = isRequest(input) ? new URL(input.url)
: input && input.href ? new URL(input.href)
: new URL(`${input}`)

if (isRequest(input)) {
init = { ...input[INTERNALS], ...init }
Expand Down Expand Up @@ -137,7 +132,7 @@ class Request extends Body {
}

get url () {
return formatUrl(this[INTERNALS].parsedURL)
return this[INTERNALS].parsedURL.toString()
}

get headers () {
Expand Down Expand Up @@ -170,10 +165,6 @@ class Request extends Body {
}

// Basic fetch
if (!parsedURL.protocol || !parsedURL.hostname) {
throw new TypeError('Only absolute URLs are supported')
}

if (!/^https?:$/.test(parsedURL.protocol)) {
throw new TypeError('Only HTTP(S) protocols are supported')
}
Expand Down Expand Up @@ -239,8 +230,21 @@ class Request extends Body {
// HTTP-network fetch step 4.2
// chunked encoding is handled by Node.js

// we cannot spread parsedURL directly, so we have to read each property one-by-one
// and map them to the equivalent https?.request() method options
const urlProps = {
auth: parsedURL.username || parsedURL.password
? `${parsedURL.username}:${parsedURL.password}`
: '',
host: parsedURL.host,
hostname: parsedURL.hostname,
path: parsedURL.pathname,
port: parsedURL.port,
protocol: parsedURL.protocol,
}

return {
...parsedURL,
...urlProps,
method: request.method,
headers: exportNodeCompatibleHeaders(headers),
agent,
Expand Down
29 changes: 22 additions & 7 deletions test/index.js
View file
Open in desktop
Expand Up @@ -24,7 +24,7 @@ const fs = require('fs')
const http = require('http')
// use of url.parse here is intentional and for coverage purposes
// eslint-disable-next-line node/no-deprecated-api
const { parse: parseURL, URLSearchParams } = require('url')
const { parse: parseURL, URLSearchParams, URL: CoreURL } = require('url')

const vm = require('vm')
const {
Expand Down Expand Up @@ -91,17 +91,21 @@ t.test('expose Headers, Response and Request constructors', t => {
t.test('support proper toString output', { skip: !supportToString }, t => {
t.equal(new Headers().toString(), '[object Headers]')
t.equal(new Response().toString(), '[object Response]')
t.equal(new Request().toString(), '[object Request]')
t.equal(new Request('http://localhost:30000').toString(), '[object Request]')
t.end()
})

t.test('reject with error if url is protocol relative', t =>
t.rejects(fetch('//example.com/'), new TypeError(
'Only absolute URLs are supported')))
t.rejects(fetch('//example.com/'), {
code: 'ERR_INVALID_URL',
name: 'TypeError',
}))

t.test('reject if url is relative path', t =>
t.rejects(fetch('/some/path'), new TypeError(
'Only absolute URLs are supported')))
t.rejects(fetch('/some/path'), {
code: 'ERR_INVALID_URL',
name: 'TypeError',
}))

t.test('reject if protocol unsupported', t =>
t.rejects(fetch('ftp://example.com/'), new TypeError(
Expand Down Expand Up @@ -1503,7 +1507,7 @@ t.test('fetch with Request instance', t => {
})
})

t.test('fetch with Node.js URL object', t => {
t.test('fetch with Node.js legacy URL object', t => {
const url = `${base}hello`
const urlObj = parseURL(url)
const req = new Request(urlObj)
Expand All @@ -1514,6 +1518,17 @@ t.test('fetch with Node.js URL object', t => {
})
})

t.test('fetch with Node.js URL object', t => {
const url = `${base}hello`
const urlObj = new CoreURL(url)
const req = new Request(urlObj)
return fetch(req).then(res => {
t.equal(res.url, url)
t.equal(res.ok, true)
t.equal(res.status, 200)
})
})

t.test('fetch with WHATWG URL object', t => {
const url = `${base}hello`
const urlObj = new URL(url)
Expand Down
63 changes: 37 additions & 26 deletions test/request.js
View file
Open in desktop
Expand Up @@ -84,7 +84,7 @@ t.test('should support wrapping Request instance', t => {
t.test('should override signal on derived Request instances', t => {
const parentAbortController = new AbortController()
const derivedAbortController = new AbortController()
const parentRequest = new Request('test', {
const parentRequest = new Request('http://localhost/test', {
signal: parentAbortController.signal,
})
const derivedRequest = new Request(parentRequest, {
Expand All @@ -97,7 +97,7 @@ t.test('should override signal on derived Request instances', t => {

t.test('should allow removing signal on derived Request instances', t => {
const parentAbortController = new AbortController()
const parentRequest = new Request(`test`, {
const parentRequest = new Request('http://localhost/test', {
signal: parentAbortController.signal,
})
const derivedRequest = new Request(parentRequest, {
Expand All @@ -109,17 +109,17 @@ t.test('should allow removing signal on derived Request instances', t => {
})

t.test('should throw error with GET/HEAD requests with body', t => {
t.throws(() => new Request('.', { body: '' }), TypeError)
t.throws(() => new Request('.', { body: 'a' }), TypeError)
t.throws(() => new Request('.', { body: '', method: 'HEAD' }), TypeError)
t.throws(() => new Request('.', { body: 'a', method: 'HEAD' }), TypeError)
t.throws(() => new Request('.', { body: 'a', method: 'get' }), TypeError)
t.throws(() => new Request('.', { body: 'a', method: 'head' }), TypeError)
t.throws(() => new Request('http://localhost', { body: '' }), TypeError)
t.throws(() => new Request('http://localhost', { body: 'a' }), TypeError)
t.throws(() => new Request('http://localhost', { body: '', method: 'HEAD' }), TypeError)
t.throws(() => new Request('http://localhost', { body: 'a', method: 'HEAD' }), TypeError)
t.throws(() => new Request('http://localhost', { body: 'a', method: 'get' }), TypeError)
t.throws(() => new Request('http://localhost', { body: 'a', method: 'head' }), TypeError)
t.end()
})

t.test('should default to null as body', t => {
const req = new Request('.')
const req = new Request(base)
t.equal(req.body, null)
return req.text().then(result => t.equal(result, ''))
})
Expand Down Expand Up @@ -194,13 +194,6 @@ t.test('should support blob() method', t => {
})
})

t.test('should support arbitrary url', t => {
const url = 'anything'
const req = new Request(url)
t.equal(req.url, 'anything')
t.end()
})

t.test('should support clone() method', t => {
const url = base
const r = new Minipass().end('a=1')
Expand Down Expand Up @@ -241,23 +234,23 @@ t.test('should support clone() method', t => {
})

t.test('should support ArrayBuffer as body', t => {
const req = new Request('', {
const req = new Request('http://localhost', {
method: 'POST',
body: stringToArrayBuffer('a=1'),
})
return req.text().then(result => t.equal(result, 'a=1'))
})

t.test('should support Uint8Array as body', t => {
const req = new Request('', {
const req = new Request('http://localhost', {
method: 'POST',
body: new Uint8Array(stringToArrayBuffer('a=1')),
})
return req.text().then(result => t.equal(result, 'a=1'))
})

t.test('should support DataView as body', t => {
const req = new Request('', {
const req = new Request('http://localhost', {
method: 'POST',
body: new DataView(stringToArrayBuffer('a=1')),
})
Expand Down Expand Up @@ -302,6 +295,18 @@ t.test('get node request options', t => {
agent: undefined,
}, 'happy path')

t.match(Request.getNodeRequestOptions(new Request('http://user:password@a.b')), {
auth: 'user:password',
}, 'sets both user and password')

t.match(Request.getNodeRequestOptions(new Request('http://user:@a.b')), {
auth: 'user:',
}, 'sets just user')

t.match(Request.getNodeRequestOptions(new Request('http://:password@a.b')), {
auth: ':password',
}, 'sets just password')

t.match(Request.getNodeRequestOptions(new Request('http://a.b', {
method: 'PATCH',
headers: {
Expand Down Expand Up @@ -349,7 +354,9 @@ t.test('get node request options', t => {
body: 'xyz',
compress: false,
})), {
href: 'http://x.y',
path: '/',
protocol: 'http:',
hostname: 'x.y',
method: 'PATCH',
headers: {
Accept: ['*/*'],
Expand All @@ -368,7 +375,9 @@ t.test('get node request options', t => {
body: new Minipass().end('xyz'),
compress: false,
})), {
href: 'http://x.y',
path: '/',
protocol: 'http:',
hostname: 'x.y',
method: 'PATCH',
headers: {
Accept: ['*/*'],
Expand All @@ -382,7 +391,9 @@ t.test('get node request options', t => {
method: 'GET',
family: 6,
})), {
href: 'http://x.y',
path: '/',
protocol: 'http:',
hostname: 'x.y',
method: 'GET',
family: 6,
})
Expand All @@ -396,7 +407,9 @@ t.test('get node request options', t => {

Request.getNodeRequestOptions(new Request('http://a.b', { agent }), {
method: 'GET',
href: 'http://a.b',
path: '/',
protocol: 'http:',
hostname: 'a.b',
agent: 420,
})

Expand All @@ -405,13 +418,11 @@ t.test('get node request options', t => {
})

t.throws(() => Request.getNodeRequestOptions(new Request('ok.html')), {
message: 'Only absolute URLs are supported',
constructor: TypeError,
code: 'ERR_INVALID_URL',
})

t.throws(() => Request.getNodeRequestOptions(new Request('xyz://ok.html')), {
message: 'Only HTTP(S) protocols are supported',
constructor: TypeError,
})

t.throws(() => Request.getNodeRequestOptions(new Request('http://a.b', {
Expand Down