TypeScript-DOM-lib-generator
TypeScript-DOM-lib-generator copied to clipboard
Convert `performance.measure()` to overloads
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:
- If startOrMeasureOptions is a non-empty
PerformanceMeasureOptions
object, run the following checks:
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.
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 😬
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')
It only throws when second argument is a non-empty object and third argument is given, it's not how overload resolution works.
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.