Skip to content

Use aOrB parameter names for union options/primitive parameters - #2577

Open
Samuel Kapust (spkapust) wants to merge 1 commit into
microsoft:mainfrom
spkapust:fix-2470-union-param-names
Open

Samuel Kapust (spkapust) wants to merge 1 commit into
microsoft:mainfrom
spkapust:fix-2470-union-param-names

Conversation

@spkapust

Copy link
Copy Markdown

Fixes #2470

Several web APIs evolved from accepting a single primitive parameter to accepting a union of that primitive and an options/arg dictionary (e.g., boolean | AddEventListenerOptions), replacing the original descriptive parameter name with a generic options or arg in WebIDL.

Following the naming convention already used in upstream WebIDL for similar APIs (such as IDBObjectStore.getAll(queryOrOptions) and Performance.measure(name, startOrMeasureOptions)), this change adds overrideName support for KDL parameter patches and renames the affected union parameters to aOrB form while preserving each spec's options vs. arg prefix:

  • EventTarget.addEventListener (and synthesized event-map overloads): options -> optionsOrUseCapture
  • EventTarget.removeEventListener (and synthesized event-map overloads): options -> optionsOrUseCapture
  • Document.importNode: options -> optionsOrDeep
  • Element.scrollIntoView: arg -> argOrAlignToTop
  • HTMLElement.togglePopover: options -> optionsOrForce
  • new SharedWorker: options -> optionsOrName

@github-actions

Copy link
Copy Markdown
Contributor

Thanks for the PR!

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

@spkapust

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree company="Google"

@saschanaz

Copy link
Copy Markdown
Contributor

I'm not sure I like the new names deviating from the spec name. I'd rather switch to multi signature if the parameter name is important. Or maybe we should add comment for parameters instead? Thoughts? Jake Bailey (@jakebailey)

@spkapust

Copy link
Copy Markdown
Author

The consensus when we last spoke about it was changing the names instead of multi signature, but happy to hear Jake's latest thoughts as well.

@jakebailey

Copy link
Copy Markdown
Member

The problem with overloads is that they behave badly when you have unions; you end up being unable to call the overloaded function with one or the other. The only fix is to then introduce more overloads.

I honestly don't quite remember where we landed way back then; I had thought you would actually be fixing your tooling rather than the types needing to be updated to make a linter happy?

@spkapust

Copy link
Copy Markdown
Author

Hi Jake,
I documented the consensus we reached during the May 4th TypeScript Friends meeting in the issue thread (#2470) . Since the consensus was to use aOrB names rather than multi-signatures, I went ahead and implemented that here .
On the tooling side, we can't resolve this in our linter because the AST only exposes the generic options parameter name. The compiler/linter has no way to infer that the boolean represents deep or useCapture without us maintaining a fragile list of hardcoded DOM exceptions (which is what we are doing now as a stopgap measure). If you want to go in a different direction let me know and I can make changes or abandon this PR.

@jakebailey

Copy link
Copy Markdown
Member

Sorry, I just didn't remember too well, May is decades ago 😄

@spkapust

Copy link
Copy Markdown
Author

No worries! Did you want to proceed with this, go in a different direction, or abandon it?

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.

Use overloads to preserve legacy parameter names when combined with modern Options objects

3 participants