Bring over last 3.9 dom changes#37502
Conversation
|
I don't think Nathan is around this eve, so adding @andrewbranch |
| interface ShareData { | ||
| text?: string; | ||
| title?: string; | ||
| url?: string; | ||
| } |
There was a problem hiding this comment.
The spec says that at least one of these properties must be present, but my hunch is it would likely be annoying to make this a union.
There was a problem hiding this comment.
Hi, I hope I'm not just adding noise here, but we recently added this type locally and tackled the same problem. This is what we came up with if it helps:
type AtLeastOneRequired<T, U = { [K in keyof T]: Pick<T, K> }> = Partial<T> & U[keyof U];
interface ShareParams {
text: string;
title: string;
url: string;
}
interface Navigator {
share?: (data: AtLeastOneRequired<ShareParams>) => Promise<void>;
}But I don't know if a goal for dom.d.ts types is to avoid complex types like AtLeastOneRequired...
There was a problem hiding this comment.
Yeah, I think that would be a goal. In this case the fully correct version is not too much to write out:
type ShareParams =
| { text: string, title?: string, url?: string }
| { text?: string, title: string, url?: string }
| { text?: string, title?: string, url: string };But I think even that may be overkill.
There was a problem hiding this comment.
Fair enough; and I would say preferred from a clarity and performance point of view.
I would prefer the union myself, but I suppose people can always do their own overrides locally like us.
There was a problem hiding this comment.
These are code-gen'd from the same source which browsers use. So for this example: https://www.w3.org/TR/web-share
Which pulls out:
partial interface Navigator {
[SecureContext] Promise<void> share(optional ShareData data = {});
};
dictionary ShareData {
USVString title;
USVString text;
USVString url;
};Which gets codegens to extend the Navigator, and create a new type:
interface ShareData {
text?: string;
title?: string;
url?: string;
}
We could make those types, but then we're not using the same source of truth as browsers
There was a problem hiding this comment.
We can change before the RC if it's really an issue. If it's concerning, maybe we have the generator declare this as a type alias instead to avoid merging and future-proof it
| msLaunchUri(uri: string, successCallback?: MSLaunchUriCallback, noHandlerCallback?: MSLaunchUriCallback): void; | ||
| requestMediaKeySystemAccess(keySystem: string, supportedConfigurations: MediaKeySystemConfiguration[]): Promise<MediaKeySystemAccess>; | ||
| sendBeacon(url: string, data?: BodyInit | null): boolean; | ||
| share(data?: ShareData): Promise<void>; |
There was a problem hiding this comment.
Should this actually be optional?
There was a problem hiding this comment.
It probably shouldn't - otherwise all other DOM apis would kinda need it, so all of them are like this
|
@orta What is this generated from? Were these changes already merged into something else? |
|
It's generated from https://github.com/microsoft/TSJS-lib-generator |
Brings in:
navigator.share