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

Simplify constructing URLSearchParams #175

Merged
merged 1 commit into from
Jan 12, 2017

Conversation

annevk
Copy link
Member

@annevk annevk commented Dec 19, 2016

Fixes #27.

@annevk annevk added needs implementer interest Moving the issue forward requires implementers to express interest needs tests Moving the issue forward requires someone to write tests labels Jan 4, 2017
@jasnell
Copy link
Collaborator

jasnell commented Jan 5, 2017

This would certainly be interesting and I would implement this in the Node.js impl if adopted in the spec. I'd like to clarify: the init param could be either a string, Iterable, or object. If Iterable, the items must be pairs of strings (USVString to be exact), and for objects, only own properties whose keys/values are strings would be considered. Is that correct?

@annevk
Copy link
Member Author

annevk commented Jan 5, 2017

Yeah. Though the USVString-ness happens through coercion, so if you have numbers there that would technically work through the magic of IDL. What's blocking this change is tests and asking browsers if they'd ship this.

@jasnell
Copy link
Collaborator

jasnell commented Jan 5, 2017

Understood. I saw the "needs implementer interest" label. Wanted to express that we would impl and ship in Node.js if added and I think it's a worthwhile change so +1 from this side. I've opened an issue to track the progress on this and will keep an eye on it.

@domenic
Copy link
Member

domenic commented Jan 5, 2017

The behavior here is governed by the definiton for how to convert a JS value into a union type. I think it ends up looking basically like this:

if (arg === null || arg === undefined) {
  // record<USVString, USVString>
  
  // add zero key/value pairs to the URLSearchParams
} else if (typeof arg === "object") {
  const iterator = arg[Symbol.iterator];
  if (iterator === undefined || iterator === null) {
    // record<USVString, USVString>
    const map = new Map();
    for (const key of Object.getOwnPropertyNames(arg)) {
      const desc = Object.getOwnPropertyDescriptor(arg, key);
      if (desc !== undefined && desc.enumerable) {
        const typedKey = toUSVString(key);
        const typedValue = toUSVString(arg[key]);
        map.set(typedKey, typedValue);
      }
    }
    
    // Use `map` to add key/value pairs to the URLSearchParams
  } else {
    // sequence<sequence<USVString>>
    const pairs = [];
    
    // Note: per spec we have to first exhaust the lists (convert
    // from JS value to sequence<sequence<USVString>>) then process them
    for (const x of iterator) {
      const gotten = [];
      pairs.push(gotten);
      for (const y of x) {
        gotten.push(toUSVString(y));
      }
    }
    
    for (const pair of pairs) {
      if (pair.length !== 2) {
        throw new TypeError();
      }
      
      // Add (`pair[0]`, `pair[1]`) key/value pair to URLSearchParams
    }
  }
} else {
  // USVString
  const string = toUSVString(arg);
  // parse and process the string to add key/value pairs to the URLSearchParams
}

@annevk
Copy link
Member Author

annevk commented Jan 5, 2017

(To be clear, it's certainly very cool that Node.js ships a compatible URL parser and API these days and your support hopefully makes for a more compelling case across the board. I didn't mean to sound dismissive.)

@annevk
Copy link
Member Author

annevk commented Jan 11, 2017

Mozilla supports this change. @mikewest, I take it Chrome would implement this too in due course?

@annevk annevk removed the needs tests Moving the issue forward requires someone to write tests label Jan 11, 2017
@annevk
Copy link
Member Author

annevk commented Jan 11, 2017

Once the tests are reviewed and landed I'll file browser issues and land this change.

@annevk annevk merged commit 31ddc5b into master Jan 12, 2017
@annevk annevk deleted the annevk/URLSearchParams-constructor branch January 12, 2017 16:20
@mikewest
Copy link
Member

Yeah. This seems totally reasonable to me. Added some flags to the bug you filed against Chromium. Thanks!

TimothyGu added a commit to TimothyGu/node that referenced this pull request Jan 28, 2017
TimothyGu added a commit to nodejs/node that referenced this pull request Feb 1, 2017
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>
TimothyGu added a commit to nodejs/node that referenced this pull request Feb 1, 2017
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>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 2, 2017
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>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 2, 2017
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>
dumganhar pushed a commit to dumganhar/chromium that referenced this pull request Mar 1, 2017
Follow up recent spec addition[1,2] and support sequence<sequence<USVString>>
initializers for URLSearchParams.

1 - whatwg/url#27
2 - whatwg/url#175

R=tyoshino,haraken
BUG=680531

Review-Url: https://codereview.chromium.org/2725593003
Cr-Commit-Position: refs/heads/master@{#453903}
TimothyGu added a commit to TimothyGu/node that referenced this pull request Apr 25, 2017
PR-URL: nodejs#12507
Ref: whatwg/url#175
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit to nodejs/node that referenced this pull request May 1, 2017
PR-URL: #12507
Ref: whatwg/url#175
Reviewed-By: James M Snell <jasnell@gmail.com>
scheib pushed a commit to scheib/chromium that referenced this pull request Jun 27, 2017
Adapt to whatwg/url#175, which dropped
URLSearchParams itself from the union accepted by the constructor. The
constructor now takes an optional

  (sequence<sequence<USVString>> or record<USVString, USVString> or USVString)

instead. Existing uses of `new URLSearchParams(<existing URLSearchParam>)`
continue to work, as per WebIDL these objects are converted to sequences
because URLSearchParams is iterable.

Intent to ship/implement thread:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/AGyufsf5XU4/CBeVLmT7BgAJ

Bug: 680531, 697378
Change-Id: Ie82d1151acc4334628be0cc258bc20e5a0dc1d15
Reviewed-on: https://chromium-review.googlesource.com/544919
Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Cr-Commit-Position: refs/heads/master@{#482588}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs implementer interest Moving the issue forward requires implementers to express interest
Development

Successfully merging this pull request may close these issues.

4 participants