Tree-Shake RemoteStore Streams #3568
Conversation
💥 No ChangesetLatest commit: 16c995f Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂 If these changes should be published to npm, you need to add a changeset. This PR includes no changesetsWhen 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 |
Binary Size ReportAffected SDKs
Test Logs |
schmidt-sebastian
force-pushed
the
mrschmidt/splitremotestore
branch
2 times, most recently
from
112b299 to
8541ba7
Compare
schmidt-sebastian
force-pushed
the
mrschmidt/splitremotestore
branch
from
8541ba7 to
ac2f643
Compare
schmidt-sebastian
requested review from
avolkovi,
davideast,
Feiyang1,
hiranya911,
hsubox76,
jamesdaniels,
sam-gc,
scottcrossen,
yuchenshi and
zwu52
as code owners
schmidt-sebastian
force-pushed
the
mrschmidt/splitremotestore
branch
from
ac2f643 to
981b8a3
Compare
schmidt-sebastian
requested review from
alikn,
andirayo,
erikeldridge,
zijianjoy and
a team
as code owners
schmidt-sebastian
changed the title
schmidt-sebastian
changed the base branch from
mrschmidt/optionalviews
to
mrschmidt/queueeverything
| * Registers a callback function that is invoked when the network status | ||
| * changes. Multiple callbacks are supported. | ||
| */ | ||
| function addNetworkStatusChangedHandler( |
There was a problem hiding this comment.
Should this just be onNetworkStatusChange to match other event handlers we have?
There was a problem hiding this comment.
I inlined this and changed the internal variable name to onNetworkStatusChange.
| * PORTING NOTE: On iOS and Android, the WatchStream gets registered on startup. | ||
| * This is not done on Web to allow it to be tree-shaken. | ||
| */ | ||
| function ensureWatchStream( |
There was a problem hiding this comment.
Optional: since this is essentially a lazy-initializing getter, you might want to just call this watchStream to cut verbosity in all the call sites.
There was a problem hiding this comment.
I left this as is since this it would require me to rename all occurrences of writeStream and watchStream (to avoid name clashes at the callsite), which might make the code even more verbose.
This PR changes the remote store to make the streams optional. In the callsites that require the streams, the streams are loaded and registered with the RemoteStore. The RemoteStore then tracks these streams via
networkStatusHandlers, which take care of forwarding the calls to disable and enable the network.This PR also ensures that the mutation queue is drained at startup if persistence is enabled. Unfortunately, this means that clients that use IndexedDb now also always depend on SyncEngine and RemoteStore.