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.

Sign up for GitHub

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

feat(firestore): Add support for multiple named databases in Firestore #2209

Merged
merged 2 commits into from
Jun 14, 2023
Merged

feat(firestore): Add support for multiple named databases in Firestore #2209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2cd3d58
Expose MultiDB within Firestore
tom-andersen Jun 9, 2023
d5a1f72
Expose MultiDB within Firestore
tom-andersen Jun 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions etc/firebase-admin.firestore.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,11 +100,20 @@ export function getFirestore(): Firestore;
// @public
export function getFirestore(app: App): Firestore;

// @beta
export function getFirestore(databaseId: string): Firestore;

// @beta
export function getFirestore(app: App, databaseId: string): Firestore;

Copy link
Member

Choose a reason for hiding this comment

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

I think if we combine the two methods the docs will be more readable.

export function getFirestore(app?: App, databaseId: string): Firestore;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think inviting customers to pass null as a parameter is a bad idea.

The problem is in doc generation that doesn't include parameters in signature:

image

Copy link
Member

Choose a reason for hiding this comment

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

What I meant to say was to keep both as optional parameters so you don't need three method signatures. It also simplifies the docs.

export function getFirestore(app?: App, databaseId?: string): Firestore;

export function getFirestore(
  appOrDatabaseId?: App | string,
  optionalDatabaseId?: string
): Firestore {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still think calling getFirestore(null, 'myDB') is worse than having overload getFirestore('myDB').

TypeScript doesn't expose the last signature, nor would I want to. The signature suggests someone could write code getFirestore('myDB', 'myDB') which isn't allowed.

export function getFirestore(
  appOrDatabaseId?: App | string,
  optionalDatabaseId?: string
): Firestore

export { GrpcStatus }

// @public
export function initializeFirestore(app: App, settings?: FirestoreSettings): Firestore;

// @beta
export function initializeFirestore(app: App, settings: FirestoreSettings, databaseId: string): Firestore;

Copy link
Member

Choose a reason for hiding this comment

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

Would it make more sense to include the databaseId in FirestoreSettings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TLDR; No, I I think this is a bad idea.

The admin SDK manages lifecycle of Firestore instances, such that subsequent requests will return the same instance. The app and databaseId form the key to this cache, and should really be held separate from settings in my opinion.

Other methods such as getFirestore, do not take a settings object. Adding a settings object here would conflict with intent to have getFirestore simply be a method to return an existing instance (if one exists).

initializeFirestore cannot be called with different settings, since settings cannot be changed after Firestore instance has been started. The semantic difference that "some" settings such as database id are allowed to change, will simply add confusion to interface. A simple rule that no settings are allowed to change can be maintained by excluding databaseId from settings.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense! Thank you

export { NestedUpdateFields }

export { OrderByDirection }
Expand Down
90 changes: 58 additions & 32 deletions src/firestore/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,58 +75,72 @@ export {
export { FirestoreSettings };

/**
* Gets the {@link https://googleapis.dev/nodejs/firestore/latest/Firestore.html | Firestore}
* Gets the default {@link https://googleapis.dev/nodejs/firestore/latest/Firestore.html | Firestore}
* service for the default app.
*
* `getFirestore()` can be called with no arguments to access the default
Copy link
Member

Choose a reason for hiding this comment

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

Please get these documentation changes reviewed by a tech writer before merging the PR. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@markarndt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Mark!

* app's `Firestore` service or as `getFirestore(app)` to access the
* `Firestore` service associated with a specific app.
*
* @example
* ```javascript
* // Get the Firestore service for the default app
* // Get the default Firestore service for the default app
* const defaultFirestore = getFirestore();
* ```

* @returns The default {@link https://googleapis.dev/nodejs/firestore/latest/Firestore.html | Firestore}
* service if no app is provided or the `Firestore` service associated with the
* provided app.
* service for the default app.
*/
export function getFirestore(): Firestore;

/**
* Gets the {@link https://googleapis.dev/nodejs/firestore/latest/Firestore.html | Firestore}
* Gets the default {@link https://googleapis.dev/nodejs/firestore/latest/Firestore.html | Firestore}
* service for the given app.
*
* `getFirestore()` can be called with no arguments to access the default
* app's `Firestore` service or as `getFirestore(app)` to access the
* `Firestore` service associated with a specific app.
*
* @example
* ```javascript
* // Get the Firestore service for a specific app
* // Get the default Firestore service for a specific app
* const otherFirestore = getFirestore(app);
* ```
*
* @param App - which `Firestore` service to
* return. If not provided, the default `Firestore` service will be returned.
* @param app - which `Firestore` service to return.
*
* @returns The default {@link https://googleapis.dev/nodejs/firestore/latest/Firestore.html | Firestore}
* service if no app is provided or the `Firestore` service associated with the
* provided app.
* service associated with the provided app.
*/
export function getFirestore(app: App): Firestore;

/**
* @param databaseId
* @internal
* Gets the named {@link https://googleapis.dev/nodejs/firestore/latest/Firestore.html | Firestore}
* service for the default app.
*
* @example
* ```javascript
* // Get the Firestore service for a named database and default app
* const otherFirestore = getFirestore('otherDb');
* ```
*
* @param databaseId - name of database to return.
*
* @returns The named {@link https://googleapis.dev/nodejs/firestore/latest/Firestore.html | Firestore}
* service for the default app.
* @beta
*/
export function getFirestore(databaseId: string): Firestore;

/**
* @param app
* @param databaseId
* @internal
* Gets the named {@link https://googleapis.dev/nodejs/firestore/latest/Firestore.html | Firestore}
* service for the given app.
*
* @example
* ```javascript
* // Get the Firestore service for a named database and specific app.
* const otherFirestore = getFirestore('otherDb');
* ```
*
* @param app - which `Firestore` service to return.
*
* @param databaseId - name of database to return.
*
* @returns The named {@link https://googleapis.dev/nodejs/firestore/latest/Firestore.html | Firestore}
* service associated with the provided app.
* @beta
*/
export function getFirestore(app: App, databaseId: string): Firestore;

Expand All @@ -144,7 +158,7 @@ export function getFirestore(
}

/**
* Gets the {@link https://googleapis.dev/nodejs/firestore/latest/Firestore.html | Firestore}
* Gets the default {@link https://googleapis.dev/nodejs/firestore/latest/Firestore.html | Firestore}
* service for the given app, passing extra parameters to its constructor.
*
* @example
Expand All @@ -153,20 +167,32 @@ export function getFirestore(
* const otherFirestore = initializeFirestore(app, {preferRest: true});
* ```
*
* @param App - which `Firestore` service to
* return. If not provided, the default `Firestore` service will be returned.
*
* @param app - which `Firestore` service to return.
*
* @param settings - Settings object to be passed to the constructor.
*
* @returns The `Firestore` service associated with the provided app and settings.
* @returns The default `Firestore` service associated with the provided app and settings.
*/
export function initializeFirestore(app: App, settings?: FirestoreSettings): Firestore;

/**
* @param app
* @param settings
* @param databaseId
* @internal
* Gets the named {@link https://googleapis.dev/nodejs/firestore/latest/Firestore.html | Firestore}
* service for the given app, passing extra parameters to its constructor.
*
* @example
* ```javascript
* // Get the Firestore service for a specific app, require HTTP/1.1 REST transport
* const otherFirestore = initializeFirestore(app, {preferRest: true}, 'otherDb');
* ```
*
* @param app - which `Firestore` service to return.
*
* @param settings - Settings object to be passed to the constructor.
*
* @param databaseId - name of database to return.
*
* @returns The named `Firestore` service associated with the provided app and settings.
* @beta
*/
export function initializeFirestore(
app: App,
Expand Down