url: extend URLSearchParams constructor #11060

TimothyGu · 2017-01-28T20:54:38Z

In addition to URLSearchParams and USVString, support record<USVString, USVString> and sequence<sequence<USVString>> also.

Fixes: #10635
Refs: whatwg/url#175
Refs: web-platform-tests/wpt#4523

CI: https://ci.nodejs.org/job/node-test-pull-request/6089/

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

url

Fixes: nodejs#10635 Refs: whatwg/url#175 Refs: web-platform-tests/wpt#4523

addaleax · 2017-01-29T04:37:01Z

doc/api/url.md

+  getSingleQueryPair(2)
+]);
+console.log(params.toString());
+  // Prints 'user=abc&query=first&query=second'


The generator examples are good but I would guess that most people probably won’t be interested in that… how about wrapping it in a <details> tag? Does that work?

@addaleax, if that is the case I'd rather just remove it. I agree the second generator example is a bit obscure, and it is in fact only compatible with URLSearchParams, not with Map (referenced earlier). Though the first example does seem useful to me.

I’m fine with any of those options… maybe somebody else has a stronger opinion but otherwise just go with whatever you prefer.

Yeah I kinda can't get the merit of the second generator example at first glance. Maybe we can leave that later with a better use case as an example?

@joyeecheung, fair.

joyeecheung

Great work! Left a few comments, please take a look.

joyeecheung · 2017-01-29T09:07:51Z

lib/internal/url.js

-    if (init instanceof URLSearchParams) {
-      const childParams = init[searchParams];
-      this[searchParams] = childParams.slice();
+    if (init === null || init === undefined) {


If we have init = '' as default then init can't be undefined?

The user can do new URLSearchParams(undefined). The default is there to echo the = "" in the spec, though now I agree it's a bit pointless.

joyeecheung · 2017-01-29T09:14:13Z

lib/internal/url.js

-      const childParams = init[searchParams];
-      this[searchParams] = childParams.slice();
+    if (init === null || init === undefined) {
+      // record<USVString, USVString>


This comment can be removed I believe.

joyeecheung · 2017-01-29T09:23:57Z

test/parallel/test-whatwg-url-searchparams-constructor.js

@@ -16,23 +16,24 @@ assert.strictEqual(params + '', 'a=b');
 params = new URLSearchParams(params);
 assert.strictEqual(params + '', 'a=b');

-// URLSearchParams constructor, empty.
+// URLSearchParams constructor, no arguments
+params = new URLSearchParams();


Do we have new URLSearchParams(null) test?

Not yet, nor do we have a new URLSearchParams(undefined) test. Both are added.

joyeecheung · 2017-01-29T09:29:35Z

doc/api/url.md

+  getSingleQueryPair(2)
+]);
+console.log(params.toString());
+  // Prints 'user=abc&query=first&query=second'


Yeah I kinda can't get the merit of the second generator example at first glance. Maybe we can leave that later with a better use case as an example?

joyeecheung · 2017-01-29T09:39:16Z

test/parallel/test-whatwg-url-searchparams-constructor.js

+  const params = new URLSearchParams(val.input);
+  let i = 0;
+  for (const param of params) {
+    assert.deepStrictEqual(param, val.output[i],


How about assert.deepStrictEqual(Array.from(params), val.output, ...)?

Good idea. Fixed also.

jasnell

Woohoo! happy to see this. Left a couple of comments but otherwise LGTM

jasnell · 2017-01-30T17:50:04Z

doc/api/url.md

@@ -525,7 +525,8 @@ value returned is equivalent to that of `url.href`.
 ### Class: URLSearchParams

 The `URLSearchParams` object provides read and write access to the query of a
-`URL`.
+`URL`. It can also be used standalone with one of the four following
+constructors.


Not a fan of starting a sentence with "It can..."... perhaps something like:

The object can also be used standalone...

jasnell · 2017-01-30T17:51:07Z

doc/api/url.md

+Instantiate a new `URLSearchParams` object with a query hash map. The key and
+value of each property of `obj` are always coerced to strings.
+
+Warning: Unlike [`querystring`][] module, duplicate keys in the form of array


s/^Warning:/^*Note*:/

TimothyGu · 2017-01-31T00:59:57Z

@jasnell, done, PTAL. I also added some more examples for interoperation of URL and URLSearchParams in 25a60ad. Please take a look at that as well.

/cc @nodejs/url

jasnell · 2017-01-31T22:31:12Z

CI: https://ci.nodejs.org/job/node-test-pull-request/6141/
still lgtm

TimothyGu · 2017-02-01T07:53:36Z

Landed in cc48f21, 29f7587, and c505b81.

PR-URL: #11060 Fixes: #10635 Ref: whatwg/url#175 Ref: web-platform-tests/wpt#4523 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

PR-URL: #11060 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

PR-URL: #11060 Ref: whatwg/url#175 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

PR-URL: nodejs#11060 Fixes: nodejs#10635 Ref: whatwg/url#175 Ref: web-platform-tests/wpt#4523 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

PR-URL: nodejs#11060 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

PR-URL: nodejs#11060 Ref: whatwg/url#175 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

TimothyGu · 2017-04-19T07:46:33Z

Added backport-requested for v7.x since it doesn't look like the spec is gonna change any time soon (re #11185 (comment)).

EDIT: it was backported in #12507, pending a merge there.

leylarezayiii

Good

TimothyGu added 3 commits January 28, 2017 11:24

url: extend URLSearchParams constructor

2300a51

Fixes: nodejs#10635 Refs: whatwg/url#175 Refs: web-platform-tests/wpt#4523

tools: add MDN link for Iterable

e8a3c95

doc: document URLSearchParams constructor

f1cc4f3

nodejs-github-bot added dont-land-on-v4.x tools Issues and PRs related to the tools directory. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Jan 28, 2017

addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 29, 2017

addaleax approved these changes Jan 29, 2017

View reviewed changes

TimothyGu mentioned this pull request Jan 29, 2017

url: track status of a spec-compliant URLSearchParams #10821

Closed

11 tasks

joyeecheung reviewed Jan 29, 2017

View reviewed changes

TimothyGu added 3 commits January 29, 2017 01:40

Address comments

1fcac02

Simplify test

1bcb813

Bring back default parameter

2813063

jasnell approved these changes Jan 30, 2017

View reviewed changes

joyeecheung approved these changes Jan 30, 2017

View reviewed changes

TimothyGu added 2 commits January 30, 2017 11:54

doc: address comments

4fcb60d

doc: add a few more URL and URLSearchParams interop examples

25a60ad

TimothyGu closed this Feb 1, 2017

TimothyGu deleted the urlsearchparams-ctor branch February 1, 2017 07:53

TimothyGu added a commit that referenced this pull request Feb 1, 2017

tools: add MDN link for Iterable

29f7587

PR-URL: #11060 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

joyeecheung mentioned this pull request Feb 1, 2017

url, test: synchronize web-platform-test url tests #11079

Closed

3 tasks

italoacasas mentioned this pull request Feb 5, 2017

Proposal - 7.6.0 (Current) #11185

Merged

italoacasas added the dont-land-on-v7.x label Feb 14, 2017

italoacasas mentioned this pull request Feb 18, 2017

(v7.x backport) URL-related changes #11457

Closed

4 tasks

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

TimothyGu mentioned this pull request Apr 19, 2017

(v7.x backport) url: updates to the WHATWG URL parser #12507

Merged

4 tasks

TimothyGu added backport-requested-v7.x and removed dont-land-on-v7.x labels Apr 19, 2017

TimothyGu added backported-to-v7.x and removed backport-requested-v7.x labels Apr 27, 2017

TimothyGu mentioned this pull request May 2, 2017

new URLSearchParams([[key1, val1], [key2, val2]]) is not working #12778

Closed

leylarezayiii reviewed May 21, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

url: extend URLSearchParams constructor #11060

url: extend URLSearchParams constructor #11060

TimothyGu commented Jan 28, 2017 •

edited

Loading

addaleax Jan 29, 2017

TimothyGu Jan 29, 2017

addaleax Jan 29, 2017

joyeecheung Jan 29, 2017

TimothyGu Jan 29, 2017

joyeecheung left a comment

joyeecheung Jan 29, 2017

TimothyGu Jan 29, 2017

joyeecheung Jan 29, 2017

TimothyGu Jan 29, 2017

joyeecheung Jan 29, 2017

TimothyGu Jan 29, 2017

joyeecheung Jan 29, 2017

joyeecheung Jan 29, 2017

TimothyGu Jan 29, 2017

jasnell left a comment

jasnell Jan 30, 2017

jasnell Jan 30, 2017

TimothyGu commented Jan 31, 2017

jasnell commented Jan 31, 2017

TimothyGu commented Feb 1, 2017

TimothyGu commented Apr 19, 2017 •

edited

Loading

leylarezayiii left a comment

url: extend URLSearchParams constructor #11060

url: extend URLSearchParams constructor #11060

Conversation

TimothyGu commented Jan 28, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Jan 31, 2017

jasnell commented Jan 31, 2017

TimothyGu commented Feb 1, 2017

TimothyGu commented Apr 19, 2017 • edited Loading

leylarezayiii left a comment

Choose a reason for hiding this comment

TimothyGu commented Jan 28, 2017 •

edited

Loading

TimothyGu commented Apr 19, 2017 •

edited

Loading