Internal implementation of multiDb #1949
Conversation
tom-andersen
force-pushed
the
tomandersen/multiDb
branch
4 times, most recently
from
e65059b
to
93c8c4f
Compare
|
please take |
tom-andersen
force-pushed
the
tomandersen/multiDb
branch
from
93c8c4f
to
a14144d
Compare
Done |
There was a problem hiding this comment.
Thank you @tom-andersen ! Overall LGTM!
I left a few comments and questions.
| this.appInternal = app; | ||
| } | ||
|
|
||
| getDatabase(databaseId: string): Firestore { | ||
| let database = this.databases.get(databaseId); | ||
| if (database === undefined) { |
There was a problem hiding this comment.
| if (database === undefined) { | |
| if (typeof database === undefined) { |
There was a problem hiding this comment.
Or we could also do if (!this.databases.has(databaseId)) { just a suggestion really. I will leave it up to you. :)
There was a problem hiding this comment.
Any of these solutions are valid. But I have a preference towards existing for a couple of reasons.
- conciseness
- performance (though only a small difference, calling
hasfollowed bygetdoes consume more resources) - correctness (even if map has value, it could be 'undefined')
Just a note, using typeof will require putting quotes around undefined.
| appOrDatabaseId?: App | string, | ||
| optionalDatabaseId?: string | ||
| ): Firestore { | ||
| const app: App = typeof appOrDatabaseId === 'object' ? appOrDatabaseId : getApp(); |
There was a problem hiding this comment.
I think type of null is also object... we might want to check if app !== null as well. Alternatively, you can also move the implementations into the respective functions separately.
There was a problem hiding this comment.
They would need to bypass Typescript type checking for this to happen, since appOrDatabaseId?: App | string.
According to this doc, we should not guard against invalid types.
https://g3doc.corp.google.com/firebase/g3doc/aip/3270.md?cl=head#input-validation
src/firestore/index.ts
Outdated
| const databaseId = | ||
| typeof appOrDatabaseId === 'string' | ||
| ? appOrDatabaseId | ||
| : optionalDatabaseId || '(default)'; |
There was a problem hiding this comment.
Yes. I have imported it from Server SDK (though it is not part of public API).
src/firestore/index.ts
Outdated
| ): Firestore { | ||
| const app: App = typeof appOrDatabaseId === 'object' ? appOrDatabaseId : getApp(); | ||
| const databaseId = | ||
| typeof appOrDatabaseId === 'string' |
There was a problem hiding this comment.
Do we want to check if appOrDatabaseId is a non-empty string? We a validator util if this is needed validator.isNonEmptyString().
There was a problem hiding this comment.
Good point. I think we should consider an empty string equivalent to the default database id.
| @@ -98,23 +98,25 @@ describe('Firestore', () => { | |||
| it(`should throw given invalid app: ${ JSON.stringify(invalidApp) }`, () => { | |||
| expect(() => { | |||
| const firestoreAny: any = FirestoreService; | |||
| return new firestoreAny(invalidApp); | |||
| const firestoreService: FirestoreService = new firestoreAny(invalidApp); | |||
| return firestoreService.getDatabase('(default)') | |||
There was a problem hiding this comment.
Should we make this '(default)' a constant?
There was a problem hiding this comment.
Imported from Server SDK
| @@ -158,19 +160,6 @@ describe('Firestore', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| describe('client', () => { | |||
There was a problem hiding this comment.
Could this be a potential breaking change in the public API?
There was a problem hiding this comment.
It's a private variable of an internal class. No one should rely on this externally.
| /** | ||
| * @param app | ||
| * @param databaseId | ||
| * @internal |
There was a problem hiding this comment.
@internal will hide this from both reference docs and autocomplete (it will remove the typescript annotation). Is that what we expect here? If we only want to hide from reference docs and would like to keep autocomplete we can use @alpha instead.
There was a problem hiding this comment.
I feel safer to keep this completely hidden for the moment. We have a follow up task to expose API.
No description provided.