Skip to content

Simplify ComponentProviders #3935

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

Jump to bottom
Merged
merged 4 commits into from
Oct 15, 2020
Merged

Simplify ComponentProviders #3935

merged 4 commits into from
Oct 15, 2020

Conversation

schmidt-sebastian
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian commented Oct 13, 2020 •
edited
Loading

This PR is meant to reduce the diff for #3901

#3901 changes the legacy Firestore SDK to use enableIndexedDbPersistence or enableMultiTabIndexedDbPersistence for any call to Firestore.enablePersistence(). This allows us to only include the IndexedDbOfflineComponentProvider or MultiTabOfflineComponentProvider when needed, which results in some simplifications.

We now know that MultiTabOfflineComponentProvider is only used with multi-tab persistence. Before, MultiTabOfflineComponentProvider also needed to support non-multi-tab persistence. This already simplified the component provider, but this PR goes a bit further and removes all persistence related settings from the Component Provider configuration and instead configures each persistence provider directly at instantiation.

This PR also adds the existing canFallback logic for persistence-related errors to firestore-exp.

@changeset-bot
Copy link

changeset-bot bot commented Oct 13, 2020 •
edited
Loading

⚠️ No Changeset found

Latest commit: 2d80ff5

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

@schmidt-sebastian schmidt-sebastian force-pushed the mrschmidt/persistencefallback branch from 6afd071 to 48d814b Compare October 13, 2020 22:51
@google-oss-bot
Copy link
Contributor

google-oss-bot commented Oct 13, 2020 •
edited
Loading

Binary Size Report

Affected SDKs

  • @firebase/firestore

    Type Base (2c1764d) Head (469d314) Diff
    browser 250 kB 250 kB +323 B (+0.1%)
    esm2017 198 kB 199 kB +264 B (+0.1%)
    main 485 kB 486 kB +837 B (+0.2%)
    module 247 kB 248 kB +323 B (+0.1%)
    react-native 198 kB 199 kB +264 B (+0.1%)

  • @firebase/firestore/exp

    Type Base (2c1764d) Head (469d314) Diff
    browser 190 kB 190 kB -79 B (-0.0%)
    main 479 kB 478 kB -83 B (-0.0%)
    module 190 kB 190 kB -79 B (-0.0%)
    react-native 190 kB 190 kB -79 B (-0.0%)

  • @firebase/firestore/memory

    Type Base (2c1764d) Head (469d314) Diff
    browser 187 kB 187 kB -281 B (-0.1%)
    esm2017 149 kB 149 kB -81 B (-0.1%)
    main 358 kB 358 kB -530 B (-0.1%)
    module 185 kB 185 kB -259 B (-0.1%)
    react-native 149 kB 149 kB -81 B (-0.1%)

  • firebase

    Type Base (2c1764d) Head (469d314) Diff
    firebase-firestore.js 287 kB 287 kB +275 B (+0.1%)
    firebase-firestore.memory.js 226 kB 226 kB -287 B (-0.1%)
    firebase.js 831 kB 831 kB +275 B (+0.0%)

Test Logs

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Oct 13, 2020 •
edited
Loading

Size Analysis Report

Affected Products

No changes between base commit (2c1764d) and head commit (469d314).

Test Logs

wilhuff
wilhuff approved these changes Oct 15, 2020
View reviewed changes
Copy link
Contributor

@wilhuff wilhuff left a comment

Choose a reason for hiding this comment

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

LGTM

packages/firestore/src/local/indexeddb_persistence.ts Outdated
'If you are using `experimentalForceOwningTab:true`, make sure that only ' +
'one tab has persistence enabled at any given time.';
'Failed to obtain exclusive access to the persistence layer. To allow ' +
'shared access, multi-tab synchronization has to be enabled in all tabs.';
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems a shame to remove the helpful error message regarding experimentalForceOwningTab:true, though it seems like we should know if that's enabled and give that helpful message only when applicable.

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 was hoping to not use the "experimental" name in firestore-exp and dropped it due to the two different names. I will come up with a way to add it back.

@wilhuff wilhuff assigned schmidt-sebastian and unassigned wilhuff Oct 15, 2020
Feiyang1
Feiyang1 approved these changes Oct 15, 2020
View reviewed changes
Copy link
Member

@Feiyang1 Feiyang1 left a comment

Choose a reason for hiding this comment

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

Approve for prettier changes

@schmidt-sebastian schmidt-sebastian merged commit 1f22925 into master Oct 15, 2020
@schmidt-sebastian schmidt-sebastian deleted the mrschmidt/persistencefallback branch November 9, 2020 22:36
@firebase firebase locked and limited conversation to collaborators Nov 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Feiyang1 Feiyang1 Feiyang1 approved these changes

@wilhuff wilhuff wilhuff approved these changes

@avolkovi avolkovi Awaiting requested review from avolkovi

@bojeil-google bojeil-google Awaiting requested review from bojeil-google

@hiranya911 hiranya911 Awaiting requested review from hiranya911

@hsubox76 hsubox76 Awaiting requested review from hsubox76

@sam-gc sam-gc Awaiting requested review from sam-gc

Assignees

@schmidt-sebastian schmidt-sebastian

Labels
api: firestore
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@schmidt-sebastian @google-oss-bot @Feiyang1 @wilhuff