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

Convert performance.measure() to overloads #1280

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

yume-chan
Copy link
Contributor

Fixes #1239

Technically the third one includes the first one (when both arguments are omitted), but if the first one is removed I feel like it's hard to discover it.

The measureOptions can't be used with endMark is defined at https://w3c.github.io/user-timing/#dom-performance-measure:

  1. If startOrMeasureOptions is a non-empty PerformanceMeasureOptions object, run the following checks:
    1. If endMark is given, throw a TypeError.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2022

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.

@saschanaz
Copy link
Contributor

saschanaz commented Mar 2, 2022

Sounds like the spec should be changed too 🤔

But for TypeScript I think it prefers unions over overloads to pass union arguments. @orta, has there been any changes over union arguments?

declare function foo(arg: number): any;
declare function foo(arg: string): any;

declare let arg: number | string;
foo(arg); // This fails 😬

@yume-chan
Copy link
Contributor Author

yume-chan commented Mar 3, 2022

In this case the two overloads has different argument length. performance.measure('name', { start: 'start' }, 'end') is allowed in current type definition but will result a runtime error.

The WebIDL spec reads: https://webidl.spec.whatwg.org/#idl-overloading-vs-union

When the operation has significantly different semantics for different argument types or lengths, overloading is preferred.

In this case maybe the difference is not "significant".

But the spec continues:

Even though the IDL is shorter in the second version [when using optional arguments], two distinctively different concepts are conflated in the first argument [startOrMeasureOptions]. Without overloads, the question "is [startMark] or [options] paired with [endMark]?" is much more difficult to answer without reading the method steps of the operation. This makes the second version remarkably less readable than the first.

IMO this case can fall in this situation.

I will try to make a pr upstream, but please leave this pr open.

EDIT: It's difficult to change the spec without actually affecting implementations, because it allows performance.measure('name', {}, 'end')

image

It only throws when second argument is a non-empty object and third argument is given, it's not how overload resolution works.

@yume-chan
Copy link
Contributor Author

I merged first two overloads like createImageBitmap does:

    createImageBitmap(image: ImageBitmapSource, options?: ImageBitmapOptions): Promise<ImageBitmap>;
    createImageBitmap(image: ImageBitmapSource, sx: number, sy: number, sw: number, sh: number, options?: ImageBitmapOptions): Promise<ImageBitmap>;

Although performance.measure('name') now satisfies both overloads, it seems TypeScript doesn't complain.

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.

performance.measure() overloads instead of optional arguments
2 participants