FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat(firestore): Add support for multiple named databases in Firestore by tom-andersen · Pull Request #2209 · firebase/firebase-admin-node · GitHub

/ firebase-admin-node Public

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

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

Merged
tom-andersen merged 2 commits into from
Jun 14, 2023
Conversation 11 Commits 2 Checks 0 Files changed
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 .md  (1) .ts  (1) All 2 file types selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
9 changes: 9 additions & 0 deletions etc/firebase-admin.firestore.api.md
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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;

lahirumaramba Jun 9, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

tom-andersen Jun 12, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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:

lahirumaramba Jun 14, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 {

tom-andersen Jun 14, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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;

lahirumaramba Jun 9, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

tom-andersen Jun 12, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

lahirumaramba Jun 14, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Makes sense! Thank you
export { NestedUpdateFields }

export { OrderByDirection }
Expand Down
90 changes: 58 additions & 32 deletions src/firestore/index.ts
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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

lahirumaramba Jun 9, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

tom-andersen Jun 12, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@markarndt

tom-andersen Jun 13, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Back | FazBrowse Home | New Git URL