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: URL.revokeObjectURL accepts no parameters when 1 is required #50432

Closed
KhafraDev opened this issue Oct 27, 2023 · 4 comments · Fixed by #50433
Closed

URL: URL.revokeObjectURL accepts no parameters when 1 is required #50432

KhafraDev opened this issue Oct 27, 2023 · 4 comments · Fixed by #50433
Labels
good first issue Issues that are suitable for first-time contributors. whatwg-url Issues and PRs related to the WHATWG URL implementation.

Comments

@KhafraDev
Copy link
Member

Version

n/a

Platform

n/a

Subsystem

url

What steps will reproduce the bug?

URL.revokeObjectURL()

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior? Why is that the expected behavior?

this should throw a TypeError

What do you see instead?

undefined

Additional information

node/lib/internal/url.js

Lines 1106 to 1108 in 3c1b4b3

function revokeObjectURL(url) {
bindingBlob.revokeObjectURL(`${url}`);
}

a check is needed along the lines of

node/lib/internal/url.js

Lines 772 to 774 in 3c1b4b3

if (arguments.length === 0) {
throw new ERR_MISSING_ARGS('url');
}

@KhafraDev KhafraDev added good first issue Issues that are suitable for first-time contributors. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Oct 27, 2023
DylanTet added a commit to DylanTet/node that referenced this issue Oct 27, 2023
Fixes: nodejs#50432

Added a check to see if url wasnt included as an argument which will then throw an error.
@anonrig
Copy link
Member

anonrig commented Oct 27, 2023

Why does it need to throw an error?

@targos
Copy link
Member

targos commented Oct 28, 2023

Because the parameter is not defined as optional.

@KhafraDev
Copy link
Member Author

relevant part of the webidl spec:

An argument is considered to be an optional argument if it is declared with the optional keyword. The final argument of a variadic operation is also considered to be an optional argument. Declaring an argument to be optional indicates that the argument value can be omitted when the operation is invoked. The final argument in an operation must not explicitly be declared to be optional if the operation is variadic.

Optional arguments can also have a default value specified. If the argument’s identifier is followed by a U+003D (=) and a value (matching DefaultValue), then that gives the optional argument its default value. The implicitly optional final argument of a variadic operation must not have a default value specified. The default value is the value to be assumed when the operation is called with the corresponding argument omitted.

@KhafraDev
Copy link
Member Author

from the webidl definition

[Exposed=(Window,DedicatedWorker,SharedWorker)]
partial interface URL {
  // ...
  static undefined revokeObjectURL(DOMString url);
};

there's a required param here

DylanTet added a commit to DylanTet/node that referenced this issue Oct 28, 2023
Fixes: nodejs#50432

Added a check to see if url wasnt included as an argument which will then throw an error.
DylanTet added a commit to DylanTet/node that referenced this issue Oct 29, 2023
Fixes: nodejs#50432

Added a check to see if url wasnt included as an argument which will then throw an error.
DylanTet added a commit to DylanTet/node that referenced this issue Oct 30, 2023
Fixes: nodejs#50432

Added a check to see if url wasnt included as an argument which will then throw an error.
strawberrywz added a commit to strawberrywz/node that referenced this issue Nov 26, 2023
deokjinkim pushed a commit to DylanTet/node that referenced this issue Nov 28, 2023
Added a check to see if url wasn't included as an argument
which will then throw an error.

Fixes: nodejs#50432
deokjinkim pushed a commit to DylanTet/node that referenced this issue Nov 28, 2023
Added a check to see if url wasn't included as an argument
which will then throw an error.

Fixes: nodejs#50432
@KhafraDev KhafraDev reopened this Nov 29, 2023
nodejs-github-bot pushed a commit that referenced this issue Nov 30, 2023
Added a check to see if url wasn't included as an argument
which will then throw an error.

Fixes: #50432
PR-URL: #50433
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
targos pushed a commit that referenced this issue Dec 4, 2023
Added a check to see if url wasn't included as an argument
which will then throw an error.

Fixes: #50432
PR-URL: #50433
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
richardlau pushed a commit that referenced this issue Mar 25, 2024
Added a check to see if url wasn't included as an argument
which will then throw an error.

Fixes: #50432
PR-URL: #50433
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues that are suitable for first-time contributors. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants