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

Update @firebase/database-types #1450

Closed
DimitarNestorov opened this issue Jan 2, 2019 · 4 comments
Closed

Update @firebase/database-types #1450

DimitarNestorov opened this issue Jan 2, 2019 · 4 comments

Comments

@DimitarNestorov
Copy link

Describe your environment

  • Operating System version: macOS 10.14.2
  • Browser version: N/A
  • Firebase SDK version: N/A
  • Firebase Product: database

Describe the problem

ThenableRefrence doesn't have catch, while Reference does.

push(value?: any, onComplete?: (a: Error | null) => any): ThenableReference;

push(value?: any, onComplete?: (a: Error | null) => void): Reference {

Reason for issue: firebase/firebase-admin-node#433

mikelehen pushed a commit that referenced this issue Jan 2, 2019
Since we implement .catch(), we should extend Promise.
@mikelehen
Copy link
Contributor

Just to be clear, per firebase/firebase-admin-node#433 I think the proposal here is that we make ThenableReference extend Promise instead of PromiseLike.

@DimitarNestorov
Copy link
Author

Out of curiosity, why does @firebase/database-types even exist? Why not just generate the definitions from @firebase/database?

@mikelehen
Copy link
Contributor

@DimitarNestorov I think it's mostly so we can carefully control our public surface area. There may be properties / methods on our implementation classes that are "public" for the sake of our own implementation details, but which aren't actually part of our supported API surface area for third-parties to use. So we use the d.ts files to clearly define (and document) our public APIs. Additionally, any changes to our public surface area are carefully reviewed and subject to extra processes, etc. so having them live in a dedicated file is helpful separation.

@mikelehen
Copy link
Contributor

I've created a PR which I think should address this. #1451

mikelehen pushed a commit that referenced this issue Jan 9, 2019
Since we implement .catch(), we should extend Promise.
@firebase firebase locked and limited conversation to collaborators Oct 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants