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

Test new URLSearchParams constructor features #4523

Merged
merged 3 commits into from
Jan 12, 2017
Merged

Test new URLSearchParams constructor features #4523

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
100f087
Test new URLSearchParams constructor features
annevk Jan 11, 2017
76ac826
address review comments
annevk Jan 11, 2017
697198f
more excellent review feedback
annevk Jan 12, 2017
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
44 changes: 39 additions & 5 deletions url/urlsearchparams-constructor.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,25 @@
}, 'Basic URLSearchParams construction');

test(function() {
assert_throws(new TypeError(), function () { URLSearchParams(); },
'Calling \'URLSearchParams\' without \'new\' should throw.');
assert_throws(new TypeError(), function () { new URLSearchParams(DOMException.prototype); });
var params = new URLSearchParams('');
var params = new URLSearchParams()
assert_equals(params.toString(), "")
}, "URLSearchParams constructor, no arguments")

test(() => {
params = new URLSearchParams(DOMException.prototype);
assert_equals(params.toString(), "Error=")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you clarify why this is the expected result here. My interpretation of the Web IDL spec [1] is that we should go to step 11.1 and treat the input as a sequence (not a String).

[1] https://heycam.github.io/webidl/#es-union

Copy link
Member

Choose a reason for hiding this comment

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

Yes but method is undefined so it then continues on with the rest of the steps.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about 11.4 ? We do have a record in there.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah you're right. I think this is indeed incorrect.

Copy link
Member Author

Choose a reason for hiding this comment

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

#4581 has the fix. I also commented on the WebKit bug.

}, "URLSearchParams constructor, DOMException.prototype as argument")

test(() => {
params = new URLSearchParams('');
assert_true(params != null, 'constructor returned non-null value.');
assert_equals(params.__proto__, URLSearchParams.prototype, 'expected URLSearchParams.prototype as prototype.');
}, "URLSearchParams constructor, empty string as argument")

test(() => {
params = new URLSearchParams({});
assert_equals(params + '', '%5Bobject+Object%5D=');
Copy link
Member

Choose a reason for hiding this comment

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

I think this should now be equivalent to no arguments per record semantics. So just empty string.

}, 'URLSearchParams constructor, empty.');
}, 'URLSearchParams constructor, {} as argument');

test(function() {
var params = new URLSearchParams('a=b');
Expand Down Expand Up @@ -124,6 +134,30 @@
params = new URLSearchParams('a%f0%9f%92%a9b=c');
assert_equals(params.get('a\uD83D\uDCA9b'), 'c');
}, 'Parse %f0%9f%92%a9'); // Unicode Character 'PILE OF POO' (U+1F4A9)

;[
{ "input": {"+": "%C2"}, "output": [[" ", "\uFFFD"]], "name": "object with +" },
Copy link
Member

Choose a reason for hiding this comment

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

@annevk, I'm in the process of implementing the URL spec change in Node.js. Where does it say in the URL Standard that keys and values of a record must be unescaped?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good question, it doesn't and I'm not sure it should. I filed whatwg/url#220.

{ "input": {c: "x", a: "?"}, "output": [["c", "x"], ["a", "?"]], "name": "object with two keys" },
{ "input": [["c", "x"], ["a", "?"]], "output": [["c", "x"], ["a", "?"]], "name": "array with two keys" }
].forEach((val) => {
test(() => {
let params = new URLSearchParams(val.input),
i = 0
for (let param of params) {
assert_array_equals(param, val.output[i])
i++
}
}, "Construct with " + val.name)
})

test(() => {
params = new URLSearchParams()
params[Symbol.iterator] = function *() {
yield ["a", "b"]
}
let params2 = new URLSearchParams(params)
assert_equals(params2.get("a"), "b")
}, "Evil constructor test")
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this works as expected. params1[Symbol.iterator] === params2[Symbol.iterator] === URLSearchParams.prototype[Symbol.iterator]. The assignment is a no-op. The iterator will look up the private data on the instance it's invoked on, so iterating over params2 will always give no params, so params3 should be empty.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, shit. I based that on #4240 which I also wrote. How would I create a custom iterator? Would assigning an array be sufficient?

Copy link
Member

Choose a reason for hiding this comment

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

The easiest way is to use a generator:

params3[Symbol.iterator] = function *() {
  yield [a, b];
  yield [c, d];
};

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, used.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe "overridden iterator test" is now more accurate.

</script>
</head>
</html>