TypeScript-DOM-lib-generator icon indicating copy to clipboard operation
TypeScript-DOM-lib-generator copied to clipboard

Convert `performance.measure()` to overloads

Open yume-chan opened this issue 2 years ago • 4 comments

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.

yume-chan avatar Mar 02 '22 03:03 yume-chan

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.

github-actions[bot] avatar Mar 02 '22 03:03 github-actions[bot]

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 😬

saschanaz avatar Mar 02 '22 16:03 saschanaz

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 avatar Mar 03 '22 02:03 yume-chan

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.

yume-chan avatar Mar 03 '22 02:03 yume-chan