Make disable internal sync and update notes about OnlineState #380
Conversation
gsoltis
requested review from
mikelehen,
schmidt-sebastian and
wilhuff
as code owners
| it('Removing default listener removes non-default listener that loads all data', function( | ||
| done | ||
| ) { | ||
| it('Removing default listener removes non-default listener that loads all data', function(done) { |
There was a problem hiding this comment.
Can you verify you have the correct prettier version (specified by yarn.lock I think) installed and it is making these changes? I think either you don't, or somebody updated yarn.lock without re-running it against the codebase.
There was a problem hiding this comment.
I've got version 1.8.2, which is what's specified in yarn.lock. Looks like the version in yarn.lock was updated most recently here: #268
There was a problem hiding this comment.
When I do a "yarn prepush" from the root of the repo, it makes a commit that undoes all of these changes... Can you give that a try?
packages/firestore/src/core/types.ts
Outdated
| /** | ||
| * Describes the online state of the Firestore client. Note that this does not | ||
| * indicate whether or not the remote store is trying to connect or not. This is | ||
| * primarily used for views determining whether or not raise events from cache. |
There was a problem hiding this comment.
So it's more nuanced than "whether or not to raise events from cache"... Maybe:
This is primarily used by the View / EventManager code to change their behavior while offline (e.g. get() calls shouldn't wait for data from the server and snapshot events should set metadata.isFromCache=true).
There was a problem hiding this comment.
Ok, I've updated the comments. Take a look.
packages/firestore/src/core/types.ts
Outdated
| export enum OnlineState { | ||
| /** | ||
| * The Firestore client is in an unknown online state. This means the client | ||
| * is either not actively trying to establish a connection or it is currently | ||
| * trying to establish a connection, but it has not succeeded or failed yet. | ||
| * Events are not raised from cache. |
There was a problem hiding this comment.
This isn't really true because snapshot listeners always get events from cache (since we'll raise a new event when we get up-to-date server data).
packages/firestore/src/core/types.ts
Outdated
| @@ -49,7 +54,7 @@ export enum OnlineState { | |||
| /** | |||
| * The client considers itself offline. It is either trying to establish a | |||
| * connection but failing, or it has been explicitly marked offline via a call | |||
| * to disableNetwork(). | |||
| * to disableNetwork(). Events will be raised from cache. | |||
There was a problem hiding this comment.
Per my previous comments, it's more nuanced than this so we should either drop this or rephrase it somehow (like maybe "Higher-level components should operate in offline mode.", but that's not that different than "The client considers itself offline.")
There was a problem hiding this comment.
I rephrased this a little, let me know if you think it's correct now.
packages/firestore/src/core/types.ts
Outdated
| * to disableNetwork(). | ||
| * The client is either trying to establish a connection but failing, or it | ||
| * has been explicitly marked offline via a call to disableNetwork(). | ||
| * Higher-level components should not operate in offline mode. |
There was a problem hiding this comment.
I think this is the opposite of correct now. :-P
There was a problem hiding this comment.
Oops, yeah, cut-and-paste fail.
| private disableNetworkInternal( | ||
| targetOnlineState: OnlineState | ||
| ): Promise<void> { | ||
| private disableNetworkInternal(targetOnlineState: OnlineState) { |
There was a problem hiding this comment.
Can you explicitly declare the return type to be void now?
|
👍 |
Tried to clarify OnlineState comments a little. Also removed the
Promise<void>return fromdisableNetworkInternal(), since it operates synchronously and was routinely being ignored anyways. Public API ofRemoteStoreremains unchanged.