Importing RTDB code via @firebase/database #112
Conversation
|
Related to #54 |
| /** | ||
| * Deletes the service and its associated resources. | ||
| * | ||
| * @return {Promise<()>} An empty Promise that will be fulfilled when the service is deleted. |
There was a problem hiding this comment.
We have been using Promise<()> in other modules (see auth.ts for example). Perhaps we keep this as it is for now, and change them all to Promise<void> in a future PR?
| let db: Database = this.databases[dbUrl]; | ||
| db.INTERNAL.delete(); | ||
| } | ||
| return Promise.resolve(undefined); |
There was a problem hiding this comment.
Same comment as above. We are currently doing Promise.resolve(undefined) in other modules.
|
|
||
| let db: Database = this.INTERNAL.databases[dbUrl]; | ||
| if (typeof db === 'undefined') { | ||
| let rtdb = require('@firebase/database'); |
There was a problem hiding this comment.
Why do you do this when you already:
import {Database} from '@firebase/database'; ?
There was a problem hiding this comment.
This is a lazy loading tactic we used with Firestore. This ensures that the RTDB implementation does not get loaded until somebody actually makes a DB call. Import statement only loads the type definition.
Plus we need to call initStandalone(), which cannot be imported like other types due to the way JS SDK exposes it.
There was a problem hiding this comment.
Not sure how lazy loading is useful in a backend server environment. Anyway, thanks for the explanation.
src/database/database.ts
Outdated
| constructor(app: FirebaseApp) { | ||
| if (!validator.isNonNullObject(app) || !('options' in app)) { | ||
| throw new FirebaseDatabaseError({ | ||
| code: 'database/invalid-argument', |
There was a problem hiding this comment.
FirebaseDatabaseError already adds the database/ prefix in the code.
test/unit/database/database.spec.ts
Outdated
|
|
||
| afterEach(() => { | ||
| return database.INTERNAL.delete().then(() => { | ||
| return mockApp.delete(); |
There was a problem hiding this comment.
nit: 2 space indent instead of 4
test/unit/database/database.spec.ts
Outdated
| it('should return a cached version of Database on subsequent calls', () => { | ||
| const db1: Database = database.getDatabase(mockApp.options.databaseURL); | ||
| const db2: Database = database.getDatabase(mockApp.options.databaseURL); | ||
| expect(db1).to.deep.equal(db2); |
There was a problem hiding this comment.
I would expect this to be equal instead of deep equal. Why are you returning different references each time?
There was a problem hiding this comment.
Done. It was just a misunderstanding on my part about the semantics of equal(). Thanks for pointing it out.
@firebase/databasemodule.database.jsfile.adminnamespace andFirebaseApp.