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

Bring over last 3.9 dom changes #37502

Merged
merged 2 commits into from
Mar 21, 2020

Conversation

orta
Copy link
Contributor

@orta orta commented Mar 20, 2020

@orta orta requested a review from sandersn March 20, 2020 20:01
@orta orta self-assigned this Mar 20, 2020
@orta orta requested a review from andrewbranch March 20, 2020 20:01
@orta
Copy link
Contributor Author

orta commented Mar 20, 2020

I don't think Nathan is around this eve, so adding @andrewbranch

Comment on lines +1700 to +1704
interface ShareData {
text?: string;
title?: string;
url?: string;
}
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.

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

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

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

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

@andrewbranch
Copy link
Member

@orta What is this generated from? Were these changes already merged into something else?

@orta
Copy link
Contributor Author

orta commented Mar 20, 2020

I agree with your feedback, but I'd like this in 3.9, happy to have that come in with the next DOM updates It'd be real tricky to get it represented as the union, see above

It's generated from https://github.com/microsoft/TSJS-lib-generator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants