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.

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

[ServerApp] RefCount FirebaseServerApps #8000

Merged
merged 4 commits into from Feb 2, 2024

Conversation

DellaBitta
Copy link
Contributor

@DellaBitta DellaBitta commented Jan 31, 2024

Discussion

Update the FirebaseServerApp creation to return the same object if an existing object exists with the same configuration. However, the deleteOnDeref field is ignored when detecting duplicate apps, since that object reference could vary across multiple SSR rendering passes.

The hope is that a FirebaseServerApp instance awaiting deletion from a the deleteOnDeref feature maybe be reused if another SSR pass occurs in rapid succession, there by speeding up the SSR code.

Testing

Updated tests to ensure that FirebaseServerApps are being deleted properly.
Added new tests to track reference counting.

API Changes

N/A.

Copy link

changeset-bot bot commented Jan 31, 2024

⚠️ No Changeset found

Latest commit: f309472

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@DellaBitta DellaBitta changed the title [ServerApp] RefCount FirebaseServerApps so they can be potentially reused across mulitple SSR passes [ServerApp] RefCount FirebaseServerApps Jan 31, 2024
@@ -235,32 +235,37 @@ export function initializeServerApp(
throw ERROR_FACTORY.create(AppError.INVALID_SERVER_APP_ENVIRONMENT);
}

if (_serverAppConfig.automaticDataCollectionEnabled === undefined) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previous detection of the reusable FirebaseServerApps wasn't taking the automaticDataCollectionEnabled field into account. We need to explicitly set this to false if the optional value is not defined, so that it aligns with future FirebaseServerApp instances that define automaticDataCollectionEnabled as explicitly false.

// TODO:
// 1: Register a new reference to finalization registry.
// 2: Incrememnt reference count.
(existingApp as FirebaseServerAppImpl).incRefCount(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The removed TODO comment of Register a new reference to finalization registry. is handled within incRefCount which, actually thinking about it, handles both of these TODOs.

@@ -362,16 +367,20 @@ export function getApps(): FirebaseApp[] {
* @public
*/
export async function deleteApp(app: FirebaseApp): Promise<void> {
let foundApp = false;
let cleanupProviders = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed this variable to make it better reflect why the code below is executed (on new line 383).

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jan 31, 2024

Size Report 1

Affected Products

  • @firebase/app

    TypeBase (abc9d87)Merge (8f716fc)Diff
    browser17.6 kB18.0 kB+451 B (+2.6%)
    esm522.9 kB23.6 kB+667 B (+2.9%)
    main24.0 kB24.7 kB+667 B (+2.8%)
    module17.6 kB18.0 kB+451 B (+2.6%)
  • firebase

    TypeBase (abc9d87)Merge (8f716fc)Diff
    firebase-app-compat.js31.1 kB31.4 kB+338 B (+1.1%)
    firebase-app.js100 kB102 kB+1.21 kB (+1.2%)
    firebase-compat.js782 kB782 kB+338 B (+0.0%)
    firebase-performance-standalone-compat.es2017.js93.0 kB93.3 kB+318 B (+0.3%)
    firebase-performance-standalone-compat.js69.9 kB70.3 kB+442 B (+0.6%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/B3EEDEnfmK.html

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jan 31, 2024

Size Analysis Report 1

Affected Products

  • @firebase/app

    • deleteApp

      Size

      TypeBase (abc9d87)Merge (8f716fc)Diff
      size8.53 kB8.56 kB+34 B (+0.4%)
      size-with-ext-deps18.0 kB18.0 kB+34 B (+0.2%)
    • initializeServerApp

      Size

      TypeBase (abc9d87)Merge (8f716fc)Diff
      size10.9 kB11.2 kB+350 B (+3.2%)
      size-with-ext-deps24.7 kB25.1 kB+350 B (+1.4%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/q4jtxaRMks.html

@DellaBitta DellaBitta marked this pull request as ready for review February 1, 2024 18:26
@DellaBitta DellaBitta requested a review from a team as a code owner February 1, 2024 18:26
Copy link
Contributor

@hsubox76 hsubox76 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 thorough, nice work.

@DellaBitta DellaBitta merged commit 6df26bb into feature-firebaseserverapp Feb 2, 2024
41 checks passed
@DellaBitta DellaBitta deleted the ddb-serverapp-refcounted branch February 2, 2024 15:40
@firebase firebase locked and limited conversation to collaborators Mar 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants