-
Notifications
You must be signed in to change notification settings - Fork 391
Allow RTDB to auto-generate typings, separate internal vs external APIs #963
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall shape looks good. But lets clean up the gulp script a bit and also figure out the discrepancy between database and database-types packages. And lets also try to narrow down the scope of the changes by keeping the FirebaseApp
module unchanged in this PR.
src/database/index.ts
Outdated
// See https://github.com/microsoft/TypeScript/issues/4336 | ||
/* eslint-disable @typescript-eslint/no-unused-vars */ | ||
// For context: github.com/typescript-eslint/typescript-eslint/issues/363 | ||
export import Database = firebaseRtdbApi.Database; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should come from the database-types package as well I think. In fact all RTDB types should come from the database-types package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good. I think there are few places where we can import our augmented Database
type instead of the FirebaseDatabase
type. Can you also comment on why the interop-types package was needed?
Thanks for the feedback. Regarding the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. LGTM 👍
Context
The goal is to bridge the gap between today's Admin SDK (manually curated
d.ts
files, nested namespaces) and a modularized SDK that has auto-generated typings.This is the first milestone from:
go/firebase-node-auto-typing
.Goals for this PR
To verify that the typings work as expected:
I also manually rannpm pack
andverify_script.sh
, however fornpm pack
it requires manual modification of thepackage.json
so that it invokes the proper gulp build script.Several concerns:
FirebaseApp
split into an interface and class (similar toApp
today)?admin
in theindex.ts
's declared namespace fordatabase
(as compared to the Auth POC) because otherwise when installingfirebase-admin
the currentindex.d.ts
wouldn't work. For example:It shouldn't be necessary to change the integration tests at all; this is because RTDB typings are using 'unknown' forval()
's return type right now. I asked Sebastian about his input on this.I imported the database types for the
DataSnapshot
, is that ok?