Skip to content

Updating API surface #527

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

Merged
merged 1 commit into from
Feb 21, 2018
Merged

Updating API surface #527

merged 1 commit into from
Feb 21, 2018

Conversation

gauntface
Copy link

@pinarx pointed out that the types file was out of sync with the API surface.

@jshcrowthe we can do away with the types files if we get the "typescript-y-ness" of the source inline and up-to-date right, or is there something I'm missing? I.e. move away from closure and opt for typescript typing

Copy link

@pinarx pinarx left a comment

Choose a reason for hiding this comment

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

thank you!

@jshcrowthe
Copy link
Contributor

jshcrowthe commented Feb 21, 2018

This PR as it is looks fine. Really the larger issue here is that the @firebase/messaging and @firebase/messaging-types packages are entirely separate. In an ideal world you would have all of your implementation code, linked to your interfaces via implements statements. Going forward there will also be cases were components will need to be able to take dependencies on the interfaces provided by other components, w/o actually importing them directly.

I'd suggest that doing that would be well worth your time.

@gauntface
Copy link
Author

Yeah we need to figure something out here to ensure it doesn't happen again.

@gauntface gauntface merged commit 3aec5ac into master Feb 21, 2018
@gauntface gauntface deleted the messaging-types-vapid branch February 21, 2018 18:16
@firebase firebase locked and limited conversation to collaborators Oct 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants