Update Webpush types in Messaging

[mmermerkaya]

Webpush Notification is a struct now: https://firebase.google.com/docs/reference/fcm/rest/v1/projects.messages#webpushconfig

[hiranya911]

@mmermerkaya thanks for putting this together. A couple of high level notes:

1. You should also update the typings in index.d.ts file.
2. We usually provide typed setters for known fields (see how the Aps type is defined). Can we do something similar here? At very least, we should do something like the following to keep backward compatibility:

{
  title?: string;
  body?: string;
  icon?: string;
  {[key: string]: any};
}

[mmermerkaya]

Thanks for the review!

TypeScript already includes Web Notification typings, but they are in the DOM library: https://github.com/Microsoft/TypeScript/blob/v2.9.1/lib/lib.dom.d.ts#L941

If we add "dom" to lib in tsconfig I can just refer to those types, and they would get updated automatically. WDYT?

[hiranya911]

Does that mean it's part of the standard TypeScript distribution? Sounds reasonable to me.

[hiranya911]

You should also use the same type def in the messaging.ts file (when declaring the WebpushConfig type)

Edit: Btw it looks like some fields are missing in NotificationOptions. I don't see title, badge, actions and a couple of others.

[mmermerkaya]

Yep, it is part of TS. Missing options are already fixed here, just not released yet.

I've done the same change in index.d.ts. I see that they are defined as types in index.d.ts and as interfaces in messaging.ts. It's recommended to use interfaces instead of types: http://go/tsstyle#interfaces-vs-type-aliases

[hiranya911]

LGTM

I'll run this by the API review team real quick before merging, since it's technically an API change. Any idea when the dom typing update will be released?

[mmermerkaya]

Ping, I think we can merge this now.

The DOM typing update will be in TS 3.0: microsoft/TypeScript-DOM-lib-generator#438 (comment)

[hiranya911]

@mmermerkaya I want to revisit the decision to extend from NotificationOptions. It seems the REST API explicitly avoided doing so since that API is likely to change, and not under our control. Can this be a concern here? Are we just better off with the following?

interface WebpushNotification {
     title?: string;
    body?: string;
    icon?: string;
    [key: string]: string;
};

[mmermerkaya]

We did that because we didn't want to keep changing our API whenever the spec changes. So we decided not to do any server side validation of the WebpushNotification payload and pass it directly to the browser (as NotificationOptions). Since TypeScript already has a NotificationOptions type that is following the spec, it makes sense to use that type here.

[hiranya911]

But that means changes in NotificationOptions can break our semver contract. Or may be it's not an issue since we already export some GCS and Firestore types in our public API as well.

@jshcrowthe what do you think?

[jshcrowthe]

I think I'm actually in favor of using the NotificationOptions interface directly rather than extend it. I'm with @mmermerkaya that the typings should be spec based and one of the spec tenants is "don't break the web" so I'm not too concerned w/ them breaking us (in such a way that we'd need a semver update).

However I think it makes sense to just call this a NotificationOptions and when the update drops we inherit the benefit. Adding a generic catchall isn't really much different than just casting a specific instance that uses properties that aren't listed to an any.

[mmermerkaya]

Extending from NotificationOptions is just a way to provide hints to the user. The API isn't actually restricted to NotificationOptions. It accepts all key-value pairs. It doesn't make sense to restrict what the user can use to NotificationOptions (which is currently not even up to date with the spec) when we explicitly decided to let the user send any payload they want. Also, title isn't part of the NotificationOptions type (because of how the API works), but it is a property that we use in the FCM SDK.

Another point is that we are not actually shipping NotificationOptions. That comes with TS, so the type will only change if the developers update their TS versions.

Casting to any is not something the user should have to do to use our API. We should not be encouraging the use of any, which is against Google TS style.

[hiranya911]

WebpushNotification is an existing type in our API so we cannot really replace it at this time. But overall this makes me feel better about reusing the existing NotificationOptions type. I will merge the PR as is shortly.

[mmermerkaya]

Ping. I can merge this myself but I want to make sure that it's okay with you.

[hiranya911]

This is causing one of our release verification scripts to fail. Looks like the users of firebase-admin now must have dom in their tsconfig. That doesn't sound right.