◐ Shell
reader mode source ↗
Skip to content

url: adding WHATWG URL support#7448

Closed
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:new-url
Closed

url: adding WHATWG URL support#7448
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:new-url

Conversation

@jasnell

@jasnell jasnell commented Jun 27, 2016 •
edited
Loading

Copy link
Copy Markdown
Member
Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

url

Description of change

This is a work in progress.

See: nodejs/node-eps#28
See: https://url.spec.whatwg.org

Implements WHATWG URL support. Example:

var u = new url.URL('http://example.org');

This is quite rough to start. Most of the WHATWG URL basic parser is supported. Currently only about 2726 of the standard tests fail. There's still quite a bit of additional work to be done on this.

@jasnell jasnell added the url label Jun 27, 2016
@jasnell jasnell added wip Issues and PRs that are still a work in progress. semver-minor PRs that contain new features and should be released in the next minor version. labels Jun 27, 2016
@jasnell

jasnell commented Jun 27, 2016

Copy link
Copy Markdown
Member Author

Related: #7355

@jasnell

jasnell commented Jun 28, 2016

Copy link
Copy Markdown
Member Author

@mscdex @bnoordhuis @indutny ... would appreciate you all taking a look. The emphasis here currently is on spec compliance but the performance definitely needs to improve. It's functionally operational but obviously can use some performance improvements. The one bit currently missing is the host parsing which is why this currently fails 26 of the standard tests (all the failures deal with host edge cases).

The WHATWG URL parsing is intentionally liberal and lenient so there are some bits we can't easily work around -- such as ignoring whitespace.

@mscdex

mscdex commented Jun 28, 2016

Copy link
Copy Markdown
Contributor

I'm curious, why implement part in javascript and part in c++?

Also, I probably wouldn't worry much about performance until it passes all tests first.

@jasnell

jasnell commented Jun 28, 2016

Copy link
Copy Markdown
Member Author

I implemented it in pure js first and the performance was fairly abysmal.
I'm planning to keep toying with the js impl to see if I can get the
performance better but this seems to do pretty well.
On Jun 27, 2016 11:03 PM, "Brian White" notifications@github.com wrote:

I'm curious, why implement part in javascript and part in c++?

Also, I probably wouldn't worry much about performance until it passes all
tests first.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#7448 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAa2ebfB63hQpDB0KxT1jE-ncZiuCQ4jks5qQLlDgaJpZM4I_c8x
.

@Fishrock123

Fishrock123 commented Jun 28, 2016

Copy link
Copy Markdown
Contributor

Not sure how widespread breakage would be, but I'm generally for this, so long as it doesn't introduce a new module or global.

@jasnell

jasnell commented Jun 28, 2016

Copy link
Copy Markdown
Member Author

Breakage would be minimal at this point because there is absolutely zero change to the existing URL implementation. Existing code should be entirely unaffected.

@Fishrock123

Fishrock123 commented Jun 29, 2016

Copy link
Copy Markdown
Contributor

Breakage would be minimal at this point because there is absolutely zero change to the existing URL implementation.

ok well, I definitely don't think we should add a new, competitive implementation.
👎

@jasnell

jasnell commented Jun 29, 2016

Copy link
Copy Markdown
Member Author

As I've said previously, the idea is to introduce the standard API in
parallel specifically to avoid breaking things. Once the impl is done and
we're sure it works, we can begin transitioning users off the old impl and
deprecate the current url.parse method or modify it so that it returns a
URL object.
On Jun 29, 2016 2:55 AM, "Jeremiah Senkpiel" notifications@github.com
wrote:

Breakage would be minimal at this point because there is absolutely zero
change to the existing URL implementation.

ok well, I definitely don't think we should add a new, competitive
implementation.
👎

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#7448 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAa2eb-DLnuTfMDY-QjSGmoS8AqS2ofxks5qQkEOgaJpZM4I_c8x
.

@btd

btd commented Jun 29, 2016

Copy link
Copy Markdown

Will it contain also URLSearchParams? It will be quite usefull, and future possible replacement for standart querystring module.

@jasnell

jasnell commented Jun 29, 2016

Copy link
Copy Markdown
Member Author

Yes, though it likely would not replace the querystring module entirely.
The querystring module would likely be used to help impl URLSearchParams.
On Jun 29, 2016 7:14 AM, "Denis Bardadym" notifications@github.com wrote:

Will it contain also URLSearchParams
https://url.spec.whatwg.org/#interface-urlsearchparams? It will be
quite usefull, and future possible replacement for standart querystring
module.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#7448 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAa2eWBvagA0DQQYPKYXYb2MWGRA7ZuWks5qQn2wgaJpZM4I_c8x
.

@jasnell

jasnell commented Jun 29, 2016

Copy link
Copy Markdown
Member Author

Ok, updated with a rough implementation of the host parsing. Currently only failing 11 of the standard parsing tests... most of which deal with punycode handling. Once #7355 lands, that'll be easier to handle.

@jasnell jasnell force-pushed the new-url branch 2 times, most recently from 8e480ee to 6b0e8a0 Compare June 30, 2016 19:06
@jasnell

jasnell commented Jun 30, 2016 •
edited
Loading

Copy link
Copy Markdown
Member Author

@nodejs/collaborators @domenic ... this is now passing 100% of the parsing tests and all but two of the setter tests (those two are currently skipped, chrome fails those two also it appears). I have added the URLSearchParams impl and the URL statics domainToASCII() and domainToUnicode().

At this point, this has the appropriate test coverage but the performance is still lacking. The next step after this is to begin working on improving performance without sacrificing correctness. It's likely to not perform as well as require('url').parse() for a while but it's reasonable.

@jasnell

jasnell commented Jun 30, 2016 •
edited
Loading

Copy link
Copy Markdown
Member Author

First CI: https://ci.nodejs.org/job/node-test-pull-request/3145/
Some failures on various platforms. Will be investigating...

89 hidden items Load more…
@ChALkeR ChALkeR mentioned this pull request Feb 14, 2017
Closed
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
TimothyGu pushed a commit to TimothyGu/node that referenced this pull request Nov 28, 2017
@jasnell @TimothyGu
url: adding WHATWG URL support …
77d08f9 
Implements WHATWG URL support. Example:

```
var u = new url.URL('http://example.org');
```

Many, many other commits improving the implementation have been squashed
into this backport PR. They are not listed separately here for brevity.

PR-URL: nodejs#7448
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
MylesBorins pushed a commit that referenced this pull request Jan 18, 2018
@jasnell @MylesBorins
url: adding WHATWG URL support …
3639d0c 
Implements WHATWG URL support. Example:

```
var u = new url.URL('http://example.org');
```

Many, many other commits improving the implementation have been squashed
into this backport PR. They are not listed separately here for brevity.

Backport-PR-URL: #17365
PR-URL: #7448
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Feb 11, 2018
@jasnell @MylesBorins
url: adding WHATWG URL support …
2d088ec 
Implements WHATWG URL support. Example:

```
var u = new url.URL('http://example.org');
```

Many, many other commits improving the implementation have been squashed
into this backport PR. They are not listed separately here for brevity.

Backport-PR-URL: #17365
PR-URL: #7448
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
MylesBorins added a commit that referenced this pull request Feb 11, 2018
@MylesBorins
2018-01-13, Version 6.13.0 'Boron' (LTS) …
b5bef9c 
This LTS release comes with 109 commits, 17 of which are considered
Semver-Minor. This includes 32 which are doc related, 29 which are test
related, 8 which are build / tool related and 1 commit which updates
a dependency.

Notable Changes:

* console:
  - added console.count() and console.clear() (James M Snell)
    #12678
* crypto:
  - expose ECDH class (Bryan English)
    #8188
  - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas)
    #10209
  - warn on invalid authentication tag length (Tobias Nießen)
    #17566
* deps:
  - upgrade libuv to 1.16.1 (cjihrig)
    #16835
* dgram:
  - added socket.setMulticastInterface() (Will Young)
    #7855
* http:
  - add agent.keepSocketAlive and agent.reuseSocket as to allow
    overridable keep-alive behavior of `Agent` (Fedor Indutny)
    #13005
* lib:
  - return this from net.Socket.end() (Sam Roberts)
    #13481
* module:
  - add builtinModules api that provides list of all builtin modules in
    Node (Jon Moss)
    #16386
* net:
  - return this from getConnections() (Sam Roberts)
    #13553
* promises:
  - more robust stringification for unhandled rejections (Timothy Gu)
    #13784
* repl:
  - improve require() autocompletion (Alexey Orlenko)
    #14409
* src:
  - add openssl-system-ca-path configure option (Daniel Bevenius)
    #16790
  - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius)
    #12087
  - add process.ppid (cjihrig)
    #16839
* tls:
  - accept `lookup` option for `tls.connect()` (Fedor Indutny)
    #12839
* tools, build:
  - a new macOS installer! (JP Wesselink)
    #15179
* url:
  - WHATWG URL api support (James M Snell)
    #7448
* util:
  - add %i and %f formatting specifiers (Roman Reiss)
    #10308

PR-URL: #18342
MylesBorins pushed a commit that referenced this pull request Feb 12, 2018
@jasnell @MylesBorins
url: adding WHATWG URL support …
5bdc3bc 
Implements WHATWG URL support. Example:

```
var u = new url.URL('http://example.org');
```

Many, many other commits improving the implementation have been squashed
into this backport PR. They are not listed separately here for brevity.

Backport-PR-URL: #17365
PR-URL: #7448
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
MylesBorins added a commit that referenced this pull request Feb 12, 2018
@MylesBorins
2018-01-13, Version 6.13.0 'Boron' (LTS) …
8317be1 
This LTS release comes with 109 commits, 17 of which are considered
Semver-Minor. This includes 32 which are doc related, 29 which are test
related, 8 which are build / tool related and 1 commit which updates
a dependency.

Notable Changes:

* console:
  - added console.count() and console.clear() (James M Snell)
    #12678
* crypto:
  - expose ECDH class (Bryan English)
    #8188
  - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas)
    #10209
  - warn on invalid authentication tag length (Tobias Nießen)
    #17566
* deps:
  - upgrade libuv to 1.16.1 (cjihrig)
    #16835
* dgram:
  - added socket.setMulticastInterface() (Will Young)
    #7855
* http:
  - add agent.keepSocketAlive and agent.reuseSocket as to allow
    overridable keep-alive behavior of `Agent` (Fedor Indutny)
    #13005
* lib:
  - return this from net.Socket.end() (Sam Roberts)
    #13481
* module:
  - add builtinModules api that provides list of all builtin modules in
    Node (Jon Moss)
    #16386
* net:
  - return this from getConnections() (Sam Roberts)
    #13553
* promises:
  - more robust stringification for unhandled rejections (Timothy Gu)
    #13784
* repl:
  - improve require() autocompletion (Alexey Orlenko)
    #14409
* src:
  - add openssl-system-ca-path configure option (Daniel Bevenius)
    #16790
  - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius)
    #12087
  - add process.ppid (cjihrig)
    #16839
* tls:
  - accept `lookup` option for `tls.connect()` (Fedor Indutny)
    #12839
* tools, build:
  - a new macOS installer! (JP Wesselink)
    #15179
* url:
  - WHATWG URL api support (James M Snell)
    #7448
* util:
  - add %i and %f formatting specifiers (Roman Reiss)
    #10308

PR-URL: #18342
MylesBorins pushed a commit that referenced this pull request Feb 13, 2018
@jasnell @MylesBorins
url: adding WHATWG URL support …
24def19 
Implements WHATWG URL support. Example:

```
var u = new url.URL('http://example.org');
```

Many, many other commits improving the implementation have been squashed
into this backport PR. They are not listed separately here for brevity.

Backport-PR-URL: #17365
PR-URL: #7448
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
MylesBorins added a commit that referenced this pull request Feb 13, 2018
@MylesBorins
2018-01-13, Version 6.13.0 'Boron' (LTS) …
0f7dcba 
This LTS release comes with 109 commits, 17 of which are considered
Semver-Minor. This includes 32 which are doc related, 29 which are test
related, 8 which are build / tool related and 1 commit which updates
a dependency.

Notable Changes:

* console:
  - added console.count() and console.clear() (James M Snell)
    #12678
* crypto:
  - expose ECDH class (Bryan English)
    #8188
  - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas)
    #10209
  - warn on invalid authentication tag length (Tobias Nießen)
    #17566
* deps:
  - upgrade libuv to 1.16.1 (cjihrig)
    #16835
* dgram:
  - added socket.setMulticastInterface() (Will Young)
    #7855
* http:
  - add agent.keepSocketAlive and agent.reuseSocket as to allow
    overridable keep-alive behavior of `Agent` (Fedor Indutny)
    #13005
* lib:
  - return this from net.Socket.end() (Sam Roberts)
    #13481
* module:
  - add builtinModules api that provides list of all builtin modules in
    Node (Jon Moss)
    #16386
* net:
  - return this from getConnections() (Sam Roberts)
    #13553
* promises:
  - more robust stringification for unhandled rejections (Timothy Gu)
    #13784
* repl:
  - improve require() autocompletion (Alexey Orlenko)
    #14409
* src:
  - add openssl-system-ca-path configure option (Daniel Bevenius)
    #16790
  - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius)
    #12087
  - add process.ppid (cjihrig)
    #16839
* tls:
  - accept `lookup` option for `tls.connect()` (Fedor Indutny)
    #12839
* tools, build:
  - a new macOS installer! (JP Wesselink)
    #15179
* url:
  - WHATWG URL api support (James M Snell)
    #7448
* util:
  - add %i and %f formatting specifiers (Roman Reiss)
    #10308

PR-URL: #18342
MylesBorins added a commit that referenced this pull request Feb 13, 2018
@MylesBorins
2018-01-13, Version 6.13.0 'Boron' (LTS) …
9441526 
This LTS release comes with 112 commits, 17 of which are considered
Semver-Minor. This includes 32 which are doc related, 30 which are test
related, 8 which are build / tool related and 1 commit which updates
a dependency.

Notable Changes:

* console:
  - added console.count() and console.clear() (James M Snell)
    #12678
* crypto:
  - expose ECDH class (Bryan English)
    #8188
  - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas)
    #10209
  - warn on invalid authentication tag length (Tobias Nießen)
    #17566
* deps:
  - upgrade libuv to 1.16.1 (cjihrig)
    #16835
* dgram:
  - added socket.setMulticastInterface() (Will Young)
    #7855
* http:
  - add agent.keepSocketAlive and agent.reuseSocket as to allow
    overridable keep-alive behavior of `Agent` (Fedor Indutny)
    #13005
* lib:
  - return this from net.Socket.end() (Sam Roberts)
    #13481
* module:
  - add builtinModules api that provides list of all builtin modules in
    Node (Jon Moss)
    #16386
* net:
  - return this from getConnections() (Sam Roberts)
    #13553
* promises:
  - more robust stringification for unhandled rejections (Timothy Gu)
    #13784
* repl:
  - improve require() autocompletion (Alexey Orlenko)
    #14409
* src:
  - add openssl-system-ca-path configure option (Daniel Bevenius)
    #16790
  - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius)
    #12087
  - add process.ppid (cjihrig)
    #16839
* tls:
  - accept `lookup` option for `tls.connect()` (Fedor Indutny)
    #12839
* tools, build:
  - a new macOS installer! (JP Wesselink)
    #15179
* url:
  - WHATWG URL api support (James M Snell)
    #7448
* util:
  - add %i and %f formatting specifiers (Roman Reiss)
    #10308

PR-URL: #18342
MylesBorins added a commit that referenced this pull request Feb 13, 2018
@MylesBorins
2018-01-13, Version 6.13.0 'Boron' (LTS) …
60e612d 
This LTS release comes with 112 commits, 17 of which are considered
Semver-Minor. This includes 32 which are doc related, 30 which are test
related, 8 which are build / tool related and 1 commit which updates
a dependency.

Notable Changes:

* console:
  - added console.count() and console.clear() (James M Snell)
    #12678
* crypto:
  - expose ECDH class (Bryan English)
    #8188
  - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas)
    #10209
  - warn on invalid authentication tag length (Tobias Nießen)
    #17566
* deps:
  - upgrade libuv to 1.16.1 (cjihrig)
    #16835
* dgram:
  - added socket.setMulticastInterface() (Will Young)
    #7855
* http:
  - add agent.keepSocketAlive and agent.reuseSocket as to allow
    overridable keep-alive behavior of `Agent` (Fedor Indutny)
    #13005
* lib:
  - return this from net.Socket.end() (Sam Roberts)
    #13481
* module:
  - add builtinModules api that provides list of all builtin modules in
    Node (Jon Moss)
    #16386
* net:
  - return this from getConnections() (Sam Roberts)
    #13553
* promises:
  - more robust stringification for unhandled rejections (Timothy Gu)
    #13784
* repl:
  - improve require() autocompletion (Alexey Orlenko)
    #14409
* src:
  - add openssl-system-ca-path configure option (Daniel Bevenius)
    #16790
  - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius)
    #12087
  - add process.ppid (cjihrig)
    #16839
* tls:
  - accept `lookup` option for `tls.connect()` (Fedor Indutny)
    #12839
* tools, build:
  - a new macOS installer! (JP Wesselink)
    #15179
* url:
  - WHATWG URL api support (James M Snell)
    #7448
* util:
  - add %i and %f formatting specifiers (Roman Reiss)
    #10308

PR-URL: #18342
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@MylesBorins @MayaLekova
2018-01-13, Version 6.13.0 'Boron' (LTS) …
a527e80 
This LTS release comes with 112 commits, 17 of which are considered
Semver-Minor. This includes 32 which are doc related, 30 which are test
related, 8 which are build / tool related and 1 commit which updates
a dependency.

Notable Changes:

* console:
  - added console.count() and console.clear() (James M Snell)
    nodejs#12678
* crypto:
  - expose ECDH class (Bryan English)
    nodejs#8188
  - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas)
    nodejs#10209
  - warn on invalid authentication tag length (Tobias Nießen)
    nodejs#17566
* deps:
  - upgrade libuv to 1.16.1 (cjihrig)
    nodejs#16835
* dgram:
  - added socket.setMulticastInterface() (Will Young)
    nodejs#7855
* http:
  - add agent.keepSocketAlive and agent.reuseSocket as to allow
    overridable keep-alive behavior of `Agent` (Fedor Indutny)
    nodejs#13005
* lib:
  - return this from net.Socket.end() (Sam Roberts)
    nodejs#13481
* module:
  - add builtinModules api that provides list of all builtin modules in
    Node (Jon Moss)
    nodejs#16386
* net:
  - return this from getConnections() (Sam Roberts)
    nodejs#13553
* promises:
  - more robust stringification for unhandled rejections (Timothy Gu)
    nodejs#13784
* repl:
  - improve require() autocompletion (Alexey Orlenko)
    nodejs#14409
* src:
  - add openssl-system-ca-path configure option (Daniel Bevenius)
    nodejs#16790
  - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius)
    nodejs#12087
  - add process.ppid (cjihrig)
    nodejs#16839
* tls:
  - accept `lookup` option for `tls.connect()` (Fedor Indutny)
    nodejs#12839
* tools, build:
  - a new macOS installer! (JP Wesselink)
    nodejs#15179
* url:
  - WHATWG URL api support (James M Snell)
    nodejs#7448
* util:
  - add %i and %f formatting specifiers (Roman Reiss)
    nodejs#10308

PR-URL: nodejs#18342
@divyenduz divyenduz mentioned this pull request Jan 10, 2019
Closed
This was referenced Nov 10, 2024
Merged
Closed
Merged
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@targos targos targos left review comments

+1 more reviewer

@imyller imyller imyller left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. semver-minor PRs that contain new features and should be released in the next minor version. url Issues and PRs related to the legacy built-in url module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.