Skip to content

Bring over last 3.9 dom changes #37502

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Mar 21, 2020
Merged

Bring over last 3.9 dom changes #37502

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1c5c406
Bring over last 3.9 dom changes
orta Mar 20, 2020
b2498b9
Addresses feedback
orta Mar 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/lib/dom.generated.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1697,6 +1697,12 @@ interface ShadowRootInit {
mode: ShadowRootMode;
}

interface ShareData {
text?: string;
title?: string;
url?: string;
}
Comment on lines +1700 to +1704
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

@andrewbranch andrewbranch Mar 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@scvnathan scvnathan Mar 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


interface SpeechSynthesisErrorEventInit extends SpeechSynthesisEventInit {
error: SpeechSynthesisErrorCode;
}
Expand Down Expand Up @@ -10733,6 +10739,7 @@ interface Navigator extends MSFileSaver, MSNavigatorDoNotTrack, NavigatorAutomat
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>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this actually be optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably shouldn't - otherwise all other DOM apis would kinda need it, so all of them are like this

vibrate(pattern: number | number[]): boolean;
}

Expand Down Expand Up @@ -19225,7 +19232,7 @@ interface BlobCallback {
}

interface CustomElementConstructor {
new (): HTMLElement;
new (...params: any[]): HTMLElement;
}

interface DecodeErrorCallback {
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/intersectionsOfLargeUnions2.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ tests/cases/compiler/intersectionsOfLargeUnions2.ts(31,15): error TS2536: Type '
interface ElementTagNameMap {
~~~~~~~~~~~~~~~~~
!!! error TS2300: Duplicate identifier 'ElementTagNameMap'.
!!! related TS6203 /.ts/lib.dom.d.ts:19563:6: 'ElementTagNameMap' was also declared here.
!!! related TS6203 /.ts/lib.dom.d.ts:19570:6: 'ElementTagNameMap' was also declared here.
[index: number]: HTMLElement
}

Expand Down