Skip to content

Make disable internal sync and update notes about OnlineState #380

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

Merged
merged 6 commits into from
Dec 14, 2017
Merged

Make disable internal sync and update notes about OnlineState #380

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
15fe79b
Make disableNetworkInternal explicitly synchronous
Dec 13, 2017
223f5eb
Add notes to OnlineState type
Dec 14, 2017
a003e01
[AUTOMATED]: Prettier Code Styling
Dec 14, 2017
937fd0f
[AUTOMATED]: Prettier Code Styling
Dec 14, 2017
00956ef
Update comments per Michael's suggestions
Dec 14, 2017
7ffcbc9
More fixup
Dec 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions packages/database/test/order_by.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,9 +363,7 @@ describe('.orderBy tests', function() {
expect(addedPrevNames).to.deep.equal(expectedPrevNames);
});

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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

const ref = getRandomNode() as Reference;

const initial = { key: 'value' };
Expand Down
28 changes: 7 additions & 21 deletions packages/database/test/query.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1971,9 +1971,7 @@ describe('Query Tests', function() {
expect(val).to.equal(2);
});

it('.startAt() with two arguments works properly (case 1169).', function(
done
) {
it('.startAt() with two arguments works properly (case 1169).', function(done) {
const ref = getRandomNode() as Reference;
const data = {
Walker: {
Expand Down Expand Up @@ -2110,9 +2108,7 @@ describe('Query Tests', function() {
});
});

it(".endAt(null, 'f').limitToLast(5) returns the right set of children.", function(
done
) {
it(".endAt(null, 'f').limitToLast(5) returns the right set of children.", function(done) {
const ref = getRandomNode() as Reference;
ref.set(
{ a: 'a', b: 'b', c: 'c', d: 'd', e: 'e', f: 'f', g: 'g', h: 'h' },
Expand All @@ -2134,9 +2130,7 @@ describe('Query Tests', function() {
);
});

it('complex update() at query root raises correct value event', function(
done
) {
it('complex update() at query root raises correct value event', function(done) {
const nodePair = getRandomNode(2);
const writer = nodePair[0];
const reader = nodePair[1];
Expand Down Expand Up @@ -2241,9 +2235,7 @@ describe('Query Tests', function() {
});
});

it('listen for child_added events with limit and different types fires properly', function(
done
) {
it('listen for child_added events with limit and different types fires properly', function(done) {
const nodePair = getRandomNode(2);
const writer = nodePair[0];
const reader = nodePair[1];
Expand Down Expand Up @@ -2285,9 +2277,7 @@ describe('Query Tests', function() {
});
});

it('listen for child_changed events with limit and different types fires properly', function(
done
) {
it('listen for child_changed events with limit and different types fires properly', function(done) {
const nodePair = getRandomNode(2);
const writer = nodePair[0];
const reader = nodePair[1];
Expand Down Expand Up @@ -2338,9 +2328,7 @@ describe('Query Tests', function() {
});
});

it('listen for child_remove events with limit and different types fires properly', function(
done
) {
it('listen for child_remove events with limit and different types fires properly', function(done) {
const nodePair = getRandomNode(2);
const writer = nodePair[0];
const reader = nodePair[1];
Expand Down Expand Up @@ -2442,9 +2430,7 @@ describe('Query Tests', function() {
);
});

it('listen for child_remove events when parent set to scalar', function(
done
) {
it('listen for child_remove events when parent set to scalar', function(done) {
const nodePair = getRandomNode(2);
const writer = nodePair[0];
const reader = nodePair[1];
Expand Down
48 changes: 12 additions & 36 deletions packages/database/test/transaction.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,9 +86,7 @@ describe('Transaction Tests', function() {
});
});

it('Non-aborted transaction sets committed to true in callback.', function(
done
) {
it('Non-aborted transaction sets committed to true in callback.', function(done) {
const node = getRandomNode() as Reference;

node.transaction(
Expand All @@ -104,9 +102,7 @@ describe('Transaction Tests', function() {
);
});

it('Aborted transaction sets committed to false in callback.', function(
done
) {
it('Aborted transaction sets committed to false in callback.', function(done) {
const node = getRandomNode() as Reference;

node.transaction(
Expand Down Expand Up @@ -236,9 +232,7 @@ describe('Transaction Tests', function() {
return ea.promise;
});

it('Second transaction gets run immediately on previous output and only runs once.', function(
done
) {
it('Second transaction gets run immediately on previous output and only runs once.', function(done) {
const nodePair = getRandomNode(2) as Reference[];
let firstRun = false,
firstDone = false,
Expand Down Expand Up @@ -512,9 +506,7 @@ describe('Transaction Tests', function() {
);
});

it('Set should cancel already sent transactions that come back as datastale.', function(
done
) {
it('Set should cancel already sent transactions that come back as datastale.', function(done) {
const nodePair = getRandomNode(2) as Reference[];
let transactionCalls = 0;
nodePair[0].set(5, function() {
Expand Down Expand Up @@ -688,9 +680,7 @@ describe('Transaction Tests', function() {
return Promise.all([tx1, tx2]);
});

it('Doing set() in successful transaction callback works. Case 870.', function(
done
) {
it('Doing set() in successful transaction callback works. Case 870.', function(done) {
const node = getRandomNode() as Reference;
let transactionCalled = false;
let callbackCalled = false;
Expand All @@ -710,9 +700,7 @@ describe('Transaction Tests', function() {
);
});

it('Doing set() in aborted transaction callback works. Case 870.', function(
done
) {
it('Doing set() in aborted transaction callback works. Case 870.', function(done) {
const nodePair = getRandomNode(2) as Reference[],
node1 = nodePair[0],
node2 = nodePair[1];
Expand Down Expand Up @@ -1028,9 +1016,7 @@ describe('Transaction Tests', function() {
);
});

it('Transaction properly reverts data when you add a deeper listen.', function(
done
) {
it('Transaction properly reverts data when you add a deeper listen.', function(done) {
const refPair = getRandomNode(2) as Reference[],
ref1 = refPair[0],
ref2 = refPair[1];
Expand Down Expand Up @@ -1200,9 +1186,7 @@ describe('Transaction Tests', function() {
});
});

it("transaction() doesn't pick up cached data from previous once().", function(
done
) {
it("transaction() doesn't pick up cached data from previous once().", function(done) {
const refPair = getRandomNode(2) as Reference[];
const me = refPair[0],
other = refPair[1];
Expand All @@ -1229,9 +1213,7 @@ describe('Transaction Tests', function() {
});
});

it("transaction() doesn't pick up cached data from previous transaction.", function(
done
) {
it("transaction() doesn't pick up cached data from previous transaction.", function(done) {
const refPair = getRandomNode(2) as Reference[];
const me = refPair[0],
other = refPair[1];
Expand Down Expand Up @@ -1263,9 +1245,7 @@ describe('Transaction Tests', function() {
);
});

it('server values: local timestamp should eventually (but not immediately) match the server with txns', function(
done
) {
it('server values: local timestamp should eventually (but not immediately) match the server with txns', function(done) {
const refPair = getRandomNode(2) as Reference[],
writer = refPair[0],
reader = refPair[1],
Expand Down Expand Up @@ -1357,9 +1337,7 @@ describe('Transaction Tests', function() {
);
});

it("transaction() on queried location doesn't run initially on null (firebase-worker-queue depends on this).", function(
done
) {
it("transaction() on queried location doesn't run initially on null (firebase-worker-queue depends on this).", function(done) {
const ref = getRandomNode() as Reference;
ref.push({ a: 1, b: 2 }, function() {
ref
Expand Down Expand Up @@ -1437,9 +1415,7 @@ describe('Transaction Tests', function() {
);
});

it('transactions works with merges without the transaction path', function(
done
) {
it('transactions works with merges without the transaction path', function(done) {
const ref = getRandomNode() as Reference;

ref.update({ foo: 'bar' });
Expand Down
9 changes: 7 additions & 2 deletions packages/firestore/src/core/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,17 @@ export type TargetId = number;
// they're strings. We should probably (de-)serialize to a common internal type.
export type ProtoByteString = Uint8Array | string;

/** Describes the online state of the Firestore client */
/**
* 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I've updated the comments. Take a look.

*/
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

*/
Unknown,

Expand All @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.")

Copy link
Author

Choose a reason for hiding this comment

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

I rephrased this a little, let me know if you think it's correct now.

*/
Failed
}
6 changes: 3 additions & 3 deletions packages/firestore/src/platform_node/grpc_connection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,9 @@ const LOG_TAG = 'Connection';
// TODO(b/38203344): The SDK_VERSION is set independently from Firebase because
// we are doing out-of-band releases. Once we release as part of Firebase, we
// should use the Firebase version instead.
const X_GOOG_API_CLIENT_VALUE = `gl-node/${process.versions.node} fire/${
SDK_VERSION
} grpc/${grpcVersion}`;
const X_GOOG_API_CLIENT_VALUE = `gl-node/${
process.versions.node
} fire/${SDK_VERSION} grpc/${grpcVersion}`;

function createHeaders(databaseInfo: DatabaseInfo, token: Token | null): {} {
assert(
Expand Down
4 changes: 1 addition & 3 deletions packages/firestore/src/remote/persistent_stream.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,9 +261,7 @@ export abstract class PersistentStream<
// rejections are not considered unhandled.
assert(
err.code === Code.CANCELLED,
`Received unexpected error in idle timeout closure. Expected CANCELLED, but was: ${
err
}`
`Received unexpected error in idle timeout closure. Expected CANCELLED, but was: ${err}`
);
});
}
Expand Down
9 changes: 3 additions & 6 deletions packages/firestore/src/remote/remote_store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,16 +243,15 @@ export class RemoteStore {
*/
disableNetwork(): Promise<void> {
// Set the OnlineState to failed so get()'s return from cache, etc.
return this.disableNetworkInternal(OnlineState.Failed);
this.disableNetworkInternal(OnlineState.Failed);
return Promise.resolve();
}

/**
* Disables the network, setting the OnlineState to the specified
* targetOnlineState.
*/
private disableNetworkInternal(
targetOnlineState: OnlineState
): Promise<void> {
private disableNetworkInternal(targetOnlineState: OnlineState) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explicitly declare the return type to be void now?

Copy link
Author

Choose a reason for hiding this comment

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

Done.

// NOTE: We're guaranteed not to get any further events from these streams (not even a close
// event).
this.watchStream.stop();
Expand All @@ -265,8 +264,6 @@ export class RemoteStore {
this.watchStream = null;

this.updateOnlineState(targetOnlineState);

return Promise.resolve();
}

shutdown(): Promise<void> {
Expand Down
4 changes: 1 addition & 3 deletions packages/firestore/test/integration/api/validation.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,9 +172,7 @@ apiDescribe('Validation:', persistence => {
const collection = db.collection('test-collection');
const doc = collection.doc('test-document');
for (const path of badPaths) {
const reason = `Invalid path (${
path
}). Paths must not contain // in them.`;
const reason = `Invalid path (${path}). Paths must not contain // in them.`;
expect(() => db.collection(path)).to.throw(reason);
expect(() => db.doc(path)).to.throw(reason);
expect(() => collection.doc(path)).to.throw(reason);
Expand Down