Remove @firebase/messaging-types

[mmermerkaya]

Moves messaging types to an interface within the messaging package.

Closes #526.

[dwoffinden]

Isn't object the better type for "I have no idea what this is, but I don't do anything specific with it"? And that's what the implementation seems to use anyway?

firebase-js-sdk/packages/messaging/src/controllers/window-controller.ts

Line 162 in 9fd0229

nextOrObserver: NextFn<object> | Observer<object>,

[mmermerkaya]

The implementation uses object because inside the SDK we don't (and shouldn't) access any properties of the payload. Like you said, we don't do anything specific with it.

The exported types are different, because the user should know and be able to access their payload's properties.

So this would work with any and wouldn't work with object (when --strict flag is on, because of strictFunctionTypes):

messaging.onMessage((payload: UserDefinedPayloadObjectType) => {
  const messageData = payload.someField;
  // Do stuff with payload.
});

I'm planning on eventually making this a MessagePayload and preventing the --strict errors, but I'm about to propose some changes to that type, so I think it's better to keep this as any for now and change it later.

[dwoffinden]

Right, the type safety escape hatch is used because this isn't type safe, gotcha :/

Seems to me like the whole FirebaseMessaging interface should take a type parameter of the expected type, and then callers are expected to provide callbacks that handle precisely that type. In the meantime, carry on I guess :/

[jshcrowthe]

I appreciate what you are doing here. The point of the *-types packages was to allow components external to messaging to take direct dependencies on a components types w/o having to reference the actual implementation itself.

When we have done the work to better create ES Modules for our firebase components, then I think we can investigate this route a bit more. Until then, I don't think we can merge this.

[mmermerkaya]

The external components (in this case, @firebase/functions is the only one I can see) are referencing (and importing) the FirebaseMessaging interface and using it as a type. Type imports are automatically removed by TypeScript compiler. The import statement and any references to messaging are completely removed in the build, if you are worried about build sizes.

[jshcrowthe]

It's not sizing I'm concerned about at all. We want a standard for our internal component devs looking to reference an interface from another component (the actual interface, not the implementation itself). The *-types packages are that standard. I'm happy to revisit it later, but I don't think now is the right time for that.

[mmermerkaya]

Then I don't see any reason not to make the * packages that standard, instead of *-types.

[jshcrowthe]

While in principle that would work, in practice we also want to be able to version these interfaces independently from their implementations (so components can know if they are going to be broken by a change in the types). If we bake the two into the * packages you would have to create your own system for versioning types. Leveraging NPM to do this for us made more sense.

As I mentioned, once we have gone down the module path with our components (so we aren't patching FirebaseApp all the time and purposely using side-effect modules) then let's have this discussion again.

[mmermerkaya]

Components don't get broken by changes in types, types let the component developers know that the implementation changed and their code is broken.

If the types change, and their code isn't compatible with the new types, TS will let them know. That is the benefit of using TypeScript.

I don't see why anyone would need separate versions for types, or would want the types to be incompatible with the implementation, which is possible if types and implementation are imported from different packages.

[jshcrowthe]

I'm going to close this per our conversation. Let's revisit later.