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

Deprecate url.parse API in favor of URL constructor #11

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Deprecate url.parse API in favor of URL constructor #11

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
603fd51
chore: use URL constructor to parse url strings
david-luna Feb 16, 2023
363dda7
chore: add tests for double slash and auth like urls
david-luna Feb 16, 2023
279a5fa
chore: use for..in instead of list of properties
david-luna Feb 16, 2023
1fbc6b3
chohre: remove log
david-luna Feb 16, 2023
fdad8c3
fix: fix edge case which makes URL throw
david-luna Mar 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
'use strict'

const parseUrl = require('url').parse
const URL = require('url').URL
const parseForwarded = require('forwarded-parse')
const net = require('net')

module.exports = function (req) {
const raw = req.originalUrl || req.url
const url = parseUrl(raw || '')
const url = parsePartialURL(raw)
const secure = req.secure || (req.connection && req.connection.encrypted)
const result = { raw: raw }
let host
Expand Down Expand Up @@ -92,7 +92,14 @@ function getFirstHeader (req, header) {

function parsePartialURL (url) {
const containsProtocol = url.indexOf('://') !== -1
const result = parseUrl(containsProtocol ? url : 'invalid://' + url)
if (!containsProtocol) result.protocol = ''
const urlInstance = new URL(containsProtocol ? url : 'invalid://' + url)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could throw where url.parse would not throw before, e.g.:

> new URL('invalid://@@')
Uncaught TypeError [ERR_INVALID_URL]: Invalid URL
    at __node_internal_captureLargerStackTrace (node:internal/errors:478:5)
    at new NodeError (node:internal/errors:387:5)
    at URL.onParseError (node:internal/url:565:9)
    at new URL (node:internal/url:641:5) {
  input: 'invalid://@@',
  code: 'ERR_INVALID_URL'
}
> url.parse('invalid://@@')
Url {
  protocol: 'invalid:',
  slashes: true,
  auth: '@',
  host: '',
  port: null,
  hostname: '',
  hash: null,
  search: null,
  query: null,
  pathname: null,
  path: null,
  href: 'invalid://'
}

Copy link
Author

Choose a reason for hiding this comment

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

Good catch!
I think the legacy parse method is much more permissive since it consists on a loop trying to figure out as much as possible from the input string. You can look at the implementation

This means there will be more cases where the WHATWG URL API would simply throw since the input does not conform to its specs.

I guess the best approach to do not drop support is to catch the error and fallback to the legacy so we ensure support to package consumers which make use of this edge cases.

const result = {}
for (const prop in urlInstance) {
Copy link
Author

Choose a reason for hiding this comment

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

Preferred to use for..in loop to not have to maintain a list of URL properties.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I haven't run tests, but I believe you can just pass back the urlInstance here. No need to copy properties over to another object.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see that the URL instance is resistant to having the .protocol and .href changed.

> result.href = result.href.replace('invalid://', '')
Uncaught TypeError [ERR_INVALID_URL]: Invalid URL
    at __node_internal_captureLargerStackTrace (node:internal/errors:478:5)
    at new NodeError (node:internal/errors:387:5)
    at URL.onParseError (node:internal/url:565:9)
    at URL.set href [as href] (node:internal/url:755:5) {
  input: '//my/path',
  code: 'ERR_INVALID_URL'
}

What about pulling the invalid:// string out to a const INVALID_PROTO, and then in the calling code change this:

if (url.protocol) result.protocol = url.protocol

to this:

if (url.protocol !== INVALID_PROTO) result.protocol = url.protocol

Then one should be able to avoid copying the URL properties to a new object.
This could be improved upon a little bit to not be susceptible to user code actually using "invalid://" as a proto.

Copy link
Author

Choose a reason for hiding this comment

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

I just realised other parts of the logic rely on the mutability of the URL object properties to resolve some values like the protocol.

So either we complicate the main logic or we keep the property copy. Performance wise I guess 1st option would be the one to pick.

This could be improved upon a little bit to not be susceptible to user code actually using "invalid://" as a proto.

Do you mean to use a value which is longer and more unlikely to be used by consumers? Something like original-url-protocol: or something of the sorts?

Copy link
Author

Choose a reason for hiding this comment

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

Did some changes following your advice @trentm

  • changed logic to detect have a placeholder proto and detect it into the main logic. No need to copy properties
  • add a try/catch logic to fallback to legacy parse if URL throws

Something I've noticed in the tests is that parse method return more properties in the result object like slashes and auth. IMO this is a breaking change and needs to be reflected in the version with a major bump.

if (typeof urlInstance[prop] !== 'function') result[prop] = urlInstance[prop]
}
if (!containsProtocol) {
result.protocol = ''
result.href = result.href.replace('invalid://', '')
}
return result
}
56 changes: 56 additions & 0 deletions test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,34 @@ test('http - root, no special http headers', function (t) {
})
})

test('http - double slash, no special http headers', function (t) {
t.plan(1)
http({path: '//some/path'}, function (result, port) {
t.deepEqual(result, {
protocol: 'http:',
hostname: 'localhost',
port: port,
pathname: '//some/path',
full: 'http://localhost:' + port + '//some/path',
raw: '//some/path'
})
})
})

test('http - auth like, no special http headers', function (t) {
t.plan(1)
http({path: '//user@pass'}, function (result, port) {
t.deepEqual(result, {
protocol: 'http:',
hostname: 'localhost',
port: port,
pathname: '//user@pass',
full: 'http://localhost:' + port + '//user@pass',
raw: '//user@pass'
})
})
})

test('http - path, no special http headers', function (t) {
t.plan(1)
http({path: '/some/path'}, function (result, port) {
Expand Down Expand Up @@ -115,6 +143,34 @@ test('https - path, no special http headers', function (t) {
})
})

test('http - double slash, no special http headers', function (t) {
t.plan(1)
https({path: '//some/path'}, function (result, port) {
t.deepEqual(result, {
protocol: 'https:',
hostname: 'localhost',
port: port,
pathname: '//some/path',
full: 'https://localhost:' + port + '//some/path',
raw: '//some/path'
})
})
})

test('http - auth like, no special http headers', function (t) {
t.plan(1)
https({path: '//user@pass'}, function (result, port) {
t.deepEqual(result, {
protocol: 'https:',
hostname: 'localhost',
port: port,
pathname: '//user@pass',
full: 'https://localhost:' + port + '//user@pass',
raw: '//user@pass'
})
})
})

test('https - path+query params, no special http headers', function (t) {
t.plan(1)
https({path: '/some/path?key=value'}, function (result, port) {
Expand Down