Skip to content

Firestore: testing_hooks.ts: move some logic into testing_hooks_spi.ts #7543

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

Merged
merged 14 commits into from
Aug 16, 2023

Conversation

dconeybe
Copy link
Contributor

In #7149 a "testing hooks" class was added, which enables Firestore's integration tests to reach into the SDK's internals to validate behavior that is not visible from the public API surface. This original testing hooks implementation had two subtle problems:

  1. The "testing hooks" logic was unable to be tree shaken in production builds due to the SDK internals having a hard dependency on the testing hooks logic, contributing unnecessarily to the bundle size of customer applications.
  2. There was a potential for import cycles when future logic was added to the testing hooks, which causes rollup.js bundling to fail
  3. The API intended for use by the SDK internals and the API for the integration tests were both mixed together, leading to no separation and lead to potential misuse.

This PR fixes all of these issues by putting the actual testing hooks logic behind an interface. Instead of depending on the actual testing hook logic, the SDK's internals instead depend on the "SPI" (the "service provider interface"), which is basically one global variable and a bunch of TypeScript interface declarations (which do not contribute to bundle size). The integration tests, however, have a strong dependency on the testing hooks logic and bear the bundle size cost of including that code, which is fine since they're just integration tests.

This PR was motivated by #7542, which suffered from both of the problems identified above.

@dconeybe dconeybe self-assigned this Aug 10, 2023
@changeset-bot
Copy link

changeset-bot bot commented Aug 10, 2023

⚠️ No Changeset found

Latest commit: 037fc58

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

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Aug 10, 2023

Size Report 1

Affected Products

  • @firebase/firestore

    TypeBase (4ae8041)Merge (b127239)Diff
    browser371 kB371 kB+59 B (+0.0%)
    esm5356 kB356 kB+19 B (+0.0%)
    main569 kB570 kB+367 B (+0.1%)
    module371 kB371 kB+59 B (+0.0%)
    react-native371 kB371 kB+59 B (+0.0%)
  • bundle

    TypeBase (4ae8041)Merge (b127239)Diff
    firestore (Persistence)301 kB300 kB-1.12 kB (-0.4%)
    firestore (Query Cursors)239 kB238 kB-1.12 kB (-0.5%)
    firestore (Query)236 kB235 kB-1.12 kB (-0.5%)
    firestore (Read data once)224 kB223 kB-1.12 kB (-0.5%)
    firestore (Realtime updates)226 kB225 kB-1.12 kB (-0.5%)
  • firebase

    TypeBase (4ae8041)Merge (b127239)Diff
    firebase-compat.js778 kB777 kB-1.10 kB (-0.1%)
    firebase-firestore-compat.js340 kB339 kB-1.10 kB (-0.3%)
    firebase-firestore.js430 kB430 kB+59 B (+0.0%)

Test Logs

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

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Aug 10, 2023

Size Analysis Report 1

This report is too large (68,597 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.

Test Logs

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

@dconeybe dconeybe marked this pull request as ready for review August 11, 2023 17:12
@dconeybe dconeybe requested review from a team as code owners August 11, 2023 17:12
@dconeybe dconeybe changed the base branch from master to dconeybe/RunTestsInCiJsTruncatedLogOutputFix August 14, 2023 15:56
@dconeybe dconeybe changed the base branch from dconeybe/RunTestsInCiJsTruncatedLogOutputFix to master August 15, 2023 01:28
@dconeybe dconeybe merged commit 9f90a07 into master Aug 16, 2023
@dconeybe dconeybe deleted the dconeybe/TestingHooksSpi branch August 16, 2023 20:59
@firebase firebase locked and limited conversation to collaborators Sep 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants