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,lib: pass urlsearchparams-constructor.any.js #39944

Closed

Conversation

XadillaX
Copy link
Contributor

@XadillaX XadillaX commented Aug 30, 2021

According to WPT:

  1. URLSearchParams constructor should throw exactly TypeError if any
    Error occurrs.
  2. When a record passed to URLSearchParams constructor, two different
    key may result same after toUVString(). We should leave only the
    later one.

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Aug 30, 2021
@XadillaX XadillaX force-pushed the urlsearchparams-constructor.any.js branch 2 times, most recently from 64340bb to 2e3cdde Compare August 31, 2021 06:36
@XadillaX XadillaX changed the title [WIP] url,lib: pass urlsearchparams-constructor.any.js url,lib: pass urlsearchparams-constructor.any.js Aug 31, 2021
@XadillaX XadillaX force-pushed the urlsearchparams-constructor.any.js branch 2 times, most recently from 9143796 to fe1db8f Compare August 31, 2021 06:45
@@ -191,7 +209,7 @@ class URLSearchParams {
this[searchParams] = childParams.slice();
} else if (method !== null && method !== undefined) {
if (typeof method !== 'function') {
throw new ERR_ARG_NOT_ITERABLE('Query pairs');
throwTypeError(new ERR_ARG_NOT_ITERABLE('Query pairs'));
Copy link
Member

Choose a reason for hiding this comment

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

If you look at lib/internals/errors.js, you'll see that ERR_ARG_NOT_ITERABLE is already a TypeError.

@@ -201,7 +219,8 @@ class URLSearchParams {
if ((typeof pair !== 'object' && typeof pair !== 'function') ||
pair === null ||
typeof pair[SymbolIterator] !== 'function') {
throw new ERR_INVALID_TUPLE('Each query pair', '[name, value]');
throwTypeError(
new ERR_INVALID_TUPLE('Each query pair', '[name, value]'));
Copy link
Member

Choose a reason for hiding this comment

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

ERR_INVALID_TUPLE is already a TypeError

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It needs error.constructor equals to TypeError. So inheritance is not allowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I would argue that's a bug in the test harness and not our implementation.

const e = new ERR_INVALID_TUPLE('a', 'b');
console.log(e instanceof TypeError); // true

The spec text says only to throw a TypeError, and that is what we're doing here.

Copy link
Member

Choose a reason for hiding this comment

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

our internal errors already satisfy this constraint:

> try { new URLSearchParams([1]) } catch(err) { console.log(err.constructor === TypeError) }
true

Copy link
Contributor Author

@XadillaX XadillaX Aug 31, 2021

Choose a reason for hiding this comment

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

2 ways to resolve it:

  1. Ignore this case: new URLSearchParams(DOMException.prototype);
  2. Fix the code to pass wpt.

Both of the two ways are OK, but current code can't pass that case.

Copy link
Member

Choose a reason for hiding this comment

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

It was fixed in #33857

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll take a look later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But new URLSearchParams(DOMException.prototype); is thrown by DOMException.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@targos You're right. The only thing I should fix for error handling is the ERR_INVALID_THIS in lib/internal/per_context/domexception.js.


throw new TypeError(message); // eslint-disable-line
}

Copy link
Member

Choose a reason for hiding this comment

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

This is not the correct way of handling this. If you take a look at lib/internals/errors.js, you'll see that some error codes have multiple types in their list... for instance ERR_INVALID_STATE lists Error, TypeError, and RangeError. To create an ERR_INVALID_STATE error this is a TypeError, you simply do new ERR_INVALID_STATE.TypeError("message")

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

The TypeError handling in this is not correct. Haven't looked at the rest yet.

@XadillaX XadillaX force-pushed the urlsearchparams-constructor.any.js branch 2 times, most recently from 7b72a8c to 4c101fc Compare September 1, 2021 03:29
@XadillaX
Copy link
Contributor Author

XadillaX commented Sep 2, 2021

The TypeError handling in this is not correct. Haven't looked at the rest yet.

I've fixed them.

@XadillaX
Copy link
Contributor Author

XadillaX commented Sep 6, 2021

/ping @jasnell

@XadillaX
Copy link
Contributor Author

XadillaX commented Sep 7, 2021

/ping @jasnell again :)

@XadillaX
Copy link
Contributor Author

/ping @jasnell

@targos
Copy link
Member

targos commented Sep 13, 2021

@nodejs/url

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@XadillaX
Copy link
Contributor Author

I think this PR need @jasnell to approve because he gave a Change Requested.

/ping @jasnell

@XadillaX
Copy link
Contributor Author

/ping @jasnell

@@ -11,6 +11,12 @@ runner.setScriptModifier((obj) => {
// created via `document.createElement`. So we need to ignore them and just
// test `URL`.
obj.code = obj.code.replace(/\["url", "a", "area"\]/, '[ "url" ]');
} else if (obj.filename.includes('urlsearchparams-constructor.any.js')) {
// Ignore test named `URLSearchParams constructor, FormData.` because we do
// not have `FormData`.
Copy link
Member

Choose a reason for hiding this comment

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

If we decided to add FormData later someone might miss removing this. It would be worthwhile making this replacement dependent on whether FormData is undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I've add typeof FormData === 'undefined'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/ping @jasnell

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/ping @jasnell

@XadillaX XadillaX force-pushed the urlsearchparams-constructor.any.js branch from 4c101fc to 4b8ea64 Compare September 17, 2021 03:59
@nodejs-github-bot

This comment has been minimized.

According to WPT:

1. `URLSearchParams` constructor should throw exactly `TypeError` if any
   Error occurrs.
2. When a record passed to `URLSearchParams` constructor, two different
   key may result same after `toUVString()`. We should leave only the
   later one.
@XadillaX XadillaX force-pushed the urlsearchparams-constructor.any.js branch from 4b8ea64 to c2e77f2 Compare September 17, 2021 04:01
@nodejs-github-bot
Copy link
Collaborator

@XadillaX
Copy link
Contributor Author

/ping @jasnell Could you please take a minute to review this PR again? Thanks.

@XadillaX
Copy link
Contributor Author

/ping @jasnell

1 similar comment
@XadillaX
Copy link
Contributor Author

/ping @jasnell

@XadillaX
Copy link
Contributor Author

XadillaX commented Oct 3, 2021

/ping @jasnell again.

@XadillaX
Copy link
Contributor Author

XadillaX commented Oct 8, 2021

Hi @jasnell, would you please to review this PR again?

@XadillaX
Copy link
Contributor Author

/ping @jasnell

1 similar comment
@XadillaX
Copy link
Contributor Author

XadillaX commented Nov 3, 2021

/ping @jasnell

@watilde watilde added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 3, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 3, 2021
@nodejs-github-bot
Copy link
Collaborator

@XadillaX
Copy link
Contributor Author

XadillaX commented Nov 5, 2021

/ping @jasnell again

@XadillaX
Copy link
Contributor Author

/ping @jasnell

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. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants