Skip to content

Adds a rule to make sure that optional params can't come before required#2274

Closed
Bashamega wants to merge 1 commit intomicrosoft:mainfrom
Bashamega:optional
Closed

Adds a rule to make sure that optional params can't come before required#2274
Bashamega wants to merge 1 commit intomicrosoft:mainfrom
Bashamega:optional

Conversation

@Bashamega
Copy link
Copy Markdown
Contributor

No description provided.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Dec 7, 2025

Thanks for the PR!

This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged.

"param": [
{
"name": "message",
"optional": false
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This was ok because it was one-off, but it's not great to automate this nonideal workaround.

Given the IDL parameters of "required, optional, and then required" pattern, both form of the function call should be allowed:

func(required);
func(required, undefined, required);

This can be only done with overloads in current TS. I'm not sure it's worth right now given it's very rare to have this problem.

}

function convertArgument(arg: webidl2.Argument): Browser.Param {
function hasRequiredArgumentAfter(arr: webidl2.Argument[], i: number): boolean {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Also I think this is a wrong layer to solve the problem. widlprocess should be about conversion to the format that the build script understands, and not about changing anything. That should be part of the emitter, as the comment above says.

@Bashamega
Copy link
Copy Markdown
Contributor Author

Okay

@Bashamega Bashamega closed this Dec 8, 2025
@Bashamega Bashamega deleted the optional branch December 24, 2025 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants