Skip to content

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

Merged
merged 50 commits into from
Aug 12, 2020

Conversation

MathBunny
Copy link
Contributor

@MathBunny MathBunny commented Jul 22, 2020

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

  1. Allow for auto-generated typings for RTDB
  2. Split the internal and external APIs
  3. Still use the previous manually curated typings
  4. Be a non-breaking change

To verify that the typings work as expected:

export TYPE_GENERATION_MODE=auto && npm test && npm run integration && npm pack && ./.github/scripts/verify_package.sh firebase-admin-*

I also manually ran npm pack and verify_script.sh, however for npm pack it requires manual modification of the package.json so that it invokes the proper gulp build script.

Several concerns:

  1. Should I make the FirebaseApp split into an interface and class (similar to App today)?
  2. I added an extra admin in the index.ts's declared namespace for database (as compared to the Auth POC) because otherwise when installing firebase-admin the current index.d.ts wouldn't work. For example:
declare namespace admin.database {
  export import Database = _database.admin.database.Database;
  // this would have to be _database.database.Database; but right now it's _database.admin.database.Database
  // ...
}
  1. It shouldn't be necessary to change the integration tests at all; this is because RTDB typings are using 'unknown' for val()'s return type right now. I asked Sebastian about his input on this.

I imported the database types for the DataSnapshot, is that ok?

@MathBunny MathBunny changed the title WIP: Allow RTDB to auto-generate typings, separate internal vs external APIs Allow RTDB to auto-generate typings, separate internal vs external APIs Jul 23, 2020
@MathBunny MathBunny marked this pull request as ready for review July 23, 2020 19:47
Copy link
Contributor

@hiranya911 hiranya911 left a 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.

// 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;
Copy link
Contributor

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.

@MathBunny MathBunny requested a review from hiranya911 August 11, 2020 14:51
@MathBunny MathBunny assigned hiranya911 and unassigned MathBunny Aug 11, 2020
Copy link
Contributor

@hiranya911 hiranya911 left a 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?

@hiranya911 hiranya911 assigned MathBunny and unassigned hiranya911 Aug 12, 2020
@MathBunny MathBunny requested a review from hiranya911 August 12, 2020 14:50
@MathBunny MathBunny assigned hiranya911 and unassigned MathBunny Aug 12, 2020
@MathBunny
Copy link
Contributor Author

Thanks for the feedback. Regarding the interop-types, turns out I don't actually need it, thanks for pointing that out.

Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM 👍

@hiranya911 hiranya911 assigned MathBunny and unassigned hiranya911 Aug 12, 2020
@MathBunny MathBunny merged commit 222fb3b into master Aug 12, 2020
@MathBunny MathBunny deleted the hlazu-modularizing-rtdb branch August 12, 2020 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants