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

url: more closely align url.parse() with WHATWHG URL #42140

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Feb 26, 2022

There are still lots of ways they are different, but this aligns
url.parse() a bit more closely with WHATWHG URL.

This is a follow-on to #42136 but includes breaking changes.

@Trott Trott added semver-major PRs that contain breaking changes and should be released in the next major version. request-ci Add this label to start a Jenkins CI on a PR. labels Feb 26, 2022
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. url Issues and PRs related to the legacy built-in url module. labels Feb 26, 2022
@Trott
Copy link
Member Author

Trott commented Feb 26, 2022

@Trott Trott added the notable-change PRs with changes that should be highlighted in changelogs. label Feb 26, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2022
@nodejs-github-bot
Copy link
Collaborator

@lpinca
Copy link
Member

lpinca commented Feb 27, 2022

This is not related to this specific change but for non special protocols behavior should be different

> url.parse('sip:alice@atlanta.com')
Url {
  protocol: 'sip:',
  slashes: null,
  auth: 'alice',
  host: 'atlanta.com',
  port: null,
  hostname: 'atlanta.com',
  hash: null,
  search: null,
  query: null,
  pathname: null,
  path: null,
  href: 'sip:alice@atlanta.com'
}
> new URL('sip:alice@atlanta.com')
URL {
  href: 'sip:alice@atlanta.com',
  origin: 'null',
  protocol: 'sip:',
  username: '',
  password: '',
  host: '',
  hostname: '',
  port: '',
  pathname: 'alice@atlanta.com',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}

Also, there are already some other known issues:

  • https://hackerone.com/reports/771596

  • > url.parse('\x0bhttp://example.com')
    Url {
      protocol: null,
      slashes: null,
      auth: null,
      host: null,
      port: null,
      hostname: null,
      hash: null,
      search: null,
      query: null,
      pathname: '\x0Bhttp://example.com',
      path: '\x0Bhttp://example.com',
      href: '\x0Bhttp://example.com'
    }
    > new URL('\x0bhttp://example.com')
    URL {
      href: 'http://example.com/',
      origin: 'http://example.com',
      protocol: 'http:',
      username: '',
      password: '',
      host: 'example.com',
      hostname: 'example.com',
      port: '',
      pathname: '/',
      search: '',
      searchParams: URLSearchParams {},
      hash: ''
    }
    
  • > url.parse('http://@//127.0.0.1')
    Url {
      protocol: 'http:',
      slashes: true,
      auth: null,
      host: '',
      port: null,
      hostname: '',
      hash: null,
      search: null,
      query: null,
      pathname: '//127.0.0.1',
      path: '//127.0.0.1',
      href: 'http:////127.0.0.1'
    }
    > new URL('http://@//127.0.0.1')
    Uncaught TypeError [ERR_INVALID_URL]: Invalid URL
        at __node_internal_captureLargerStackTrace (node:internal/errors:465:5)
        at new NodeError (node:internal/errors:372:5)
        at onParseError (node:internal/url:563:9)
        at new URL (node:internal/url:643:5) {
      input: 'http://@//127.0.0.1',
      code: 'ERR_INVALID_URL'
    }
    

@Trott
Copy link
Member Author

Trott commented Feb 27, 2022

In its current state, this is broken enough that I'm going to move it to draft mode to avoid a raft of comments until I get it back into a more workable state.

@Trott Trott marked this pull request as draft February 27, 2022 07:49
There are still lots of ways they are different, but this aligns
url.parse() a bit more closely with WHATWHG URL.
@Trott
Copy link
Member Author

Trott commented Mar 8, 2022

I don't think this change should land so I'm going to close this.

@Trott Trott closed this Mar 8, 2022
@Trott Trott deleted the url-parse-format-again branch September 25, 2022 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. semver-major PRs that contain breaking changes and should be released in the next major version. url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants