Skip to content

Remove unused code #2817

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 5 commits into from
Mar 31, 2020
Merged

Remove unused code #2817

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fa16463
Remove some unused code
schmidt-sebastian Mar 28, 2020
e65ca11
Merge
schmidt-sebastian Mar 30, 2020
c99dba1
Merge branch 'master' into mrschmidt/byebyes
schmidt-sebastian Mar 30, 2020
4241537
Cleanup
schmidt-sebastian Mar 30, 2020
7fb6f2b
Merge
schmidt-sebastian Mar 31, 2020
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
10 changes: 1 addition & 9 deletions packages/firestore/src/core/snapshot_version.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2017 Google Inc.
* Copyright 2017 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -24,14 +24,6 @@ import { Timestamp } from '../api/timestamp';
export class SnapshotVersion {
static readonly MIN = new SnapshotVersion(new Timestamp(0, 0));

// TODO(b/34176344): Once we no longer need to use the old alpha protos,
// delete this constructor and use a timestamp-backed version everywhere.
static fromMicroseconds(value: number): SnapshotVersion {
const seconds = Math.floor(value / 1e6);
const nanos = (value % 1e6) * 1e3;
return new SnapshotVersion(new Timestamp(seconds, nanos));
}

static fromTimestamp(value: Timestamp): SnapshotVersion {
return new SnapshotVersion(value);
}
Expand Down
22 changes: 0 additions & 22 deletions packages/firestore/src/local/encoded_resource_path.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,25 +175,3 @@ export function decodeResourcePath(path: EncodedResourcePath): ResourcePath {

return new ResourcePath(segments);
}

/**
* Computes the prefix successor of the given path, computed by encode above.
* A prefix successor is the first key that cannot be prefixed by the given
* path. It's useful for defining the end of a prefix scan such that all keys
* in the scan have the same prefix.
*
* Note that this is not a general prefix successor implementation, which is
* tricky to get right with Strings, given that they encode down to UTF-8.
* Instead this relies on the fact that all paths encoded by this class are
* always terminated with a separator, and so a successor can always be
* cheaply computed by incrementing the last character of the path.
*/
export function prefixSuccessor(
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for my own information, is it acceptable to delete exported functions as long as they are not "publicly exported" in https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore-types/index.d.ts? Otherwise, wouldn't this be a backwards-incompatible change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general, if code is not in the api/ folder, it can't be used by our users.

In more technical terms, only code that is exported from our index.ts files is considered part of the public API:
https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/register-module.ts

Furthermore, the files under /api should match our types (https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore-types/index.d.ts). All code in the exported classes above should either be part of the public API or prefixed with an underscore. We don't always get this right though - see https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/src/api/database.ts#L653. logLevel is not in our types, but can be accessed since it is exported as part of Firestore.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does removing this actually help? It seems like this should have been tree-shaken out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Tree-Shaking in the browser builds doesn't actually work very well. I am trying to fix that (currently not working though - #2829), but for the "older builds" only excludes code that in modules that aren't loaded at all. Only the ESM2017 build will not include functions, but also only if they are deemed "side-effect free".

path: EncodedResourcePath
): EncodedResourcePath {
const c = path.charCodeAt(path.length - 1);
// TODO(mcg): this really should be a general thing, but not worth it right
// now
assert(c === 1, 'successor may only operate on paths generated by encode');
return path.substring(0, path.length - 1) + String.fromCharCode(c + 1);
}
33 changes: 1 addition & 32 deletions packages/firestore/src/local/local_serializer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,7 @@ import * as api from '../protos/firestore_proto_api';
import { JsonProtoSerializer } from '../remote/serializer';
import { assert, fail } from '../util/assert';
import { ByteString } from '../util/byte_string';

import { documentKeySet, DocumentKeySet } from '../model/collections';
import { Target } from '../core/target';
import {
decodeResourcePath,
encodeResourcePath,
EncodedResourcePath
} from './encoded_resource_path';
import {
DbMutationBatch,
DbNoDocument,
Expand Down Expand Up @@ -177,31 +170,7 @@ export class LocalSerializer {
mutations
);
}

/*
* Encodes a set of document keys into an array of EncodedResourcePaths.
*/
toDbResourcePaths(keys: DocumentKeySet): EncodedResourcePath[] {
const encodedKeys: EncodedResourcePath[] = [];

keys.forEach(key => {
encodedKeys.push(encodeResourcePath(key.path));
});

return encodedKeys;
}

/** Decodes an array of EncodedResourcePaths into a set of document keys. */
fromDbResourcePaths(encodedPaths: EncodedResourcePath[]): DocumentKeySet {
let keys = documentKeySet();

for (const documentKey of encodedPaths) {
keys = keys.add(new DocumentKey(decodeResourcePath(documentKey)));
}

return keys;
}


/** Decodes a DbTarget into TargetData */
fromDbTarget(dbTarget: DbTarget): TargetData {
const version = this.fromDbTimestamp(dbTarget.readTime);
Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/src/local/reference_set.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2017 Google Inc.
* Copyright 2017 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down
16 changes: 1 addition & 15 deletions packages/firestore/src/local/target_data.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2017 Google Inc.
* Copyright 2017 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -118,18 +118,4 @@ export class TargetData {
this.resumeToken
);
}

isEqual(other: TargetData): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't isEqual used indirectly by our custom deep equal mechanism?

I worry that we may have tests that are checking that something is not equal that are now accidentally passing while checking reference equality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TargetData was never passed to our custom "equals" implementation. I actually looked back at the usage and since there is only one call, I decided to inline it. So now we have no code that treats objects with isEqual and without in the same codepath.

return (
this.targetId === other.targetId &&
this.purpose === other.purpose &&
this.sequenceNumber === other.sequenceNumber &&
this.snapshotVersion.isEqual(other.snapshotVersion) &&
this.lastLimboFreeSnapshotVersion.isEqual(
other.lastLimboFreeSnapshotVersion
) &&
this.resumeToken.isEqual(other.resumeToken) &&
this.target.isEqual(other.target)
);
}
}
7 changes: 5 additions & 2 deletions packages/firestore/src/model/mutation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import { DocumentKey } from './document_key';
import { ObjectValue, ObjectValueBuilder } from './field_value';
import { FieldPath } from './path';
import { TransformOperation } from './transform_operation';
import { arrayEquals, equals } from '../util/misc';
import { arrayEquals } from '../util/misc';

/**
* Provides a set of fields that can be used to partially patch a document.
Expand Down Expand Up @@ -180,7 +180,10 @@ export class Precondition {

isEqual(other: Precondition): boolean {
return (
equals(this.updateTime, other.updateTime) && this.exists === other.exists
this.exists === other.exists &&
(this.updateTime
? !!other.updateTime && this.updateTime.isEqual(other.updateTime)
: !other.updateTime)
);
}
}
Expand Down
6 changes: 1 addition & 5 deletions packages/firestore/src/remote/existence_filter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2017 Google Inc.
* Copyright 2017 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -18,8 +18,4 @@
export class ExistenceFilter {
// TODO(b/33078163): just use simplest form of existence filter for now
constructor(public count: number) {}

isEqual(other: ExistenceFilter): boolean {
return other && other.count === this.count;
}
}
9 changes: 0 additions & 9 deletions packages/firestore/src/util/async_queue.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,15 +289,6 @@ export class AsyncQueue {
const message = error.stack || error.message || '';
logError('INTERNAL UNHANDLED ERROR: ', message);

// Escape the promise chain and throw the error globally so that
// e.g. any global crash reporting library detects and reports it.
// (but not for simulated errors in our tests since this breaks mocha)
if (message.indexOf('Firestore Test Simulated Error') < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is still used, isn't it?

firebase-js-sdk/packages/firestore/test/unit/util/async_queue.test.ts

Line 75 in bd85150

const expected = new Error('Firestore Test Simulated Error');

You mentioned in the PR description that you ran the integration tests... did you run the unit tests also? Because it seems like the unit tests should fail with this line removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is special handling of test code that is no longer needed, since we are not using mocha anymore (karma runs our browser tests). So the same test passes now without this special handling.

As for running the unit tests - all tests run as part of CI, and I usually run them locally as well. There are some tests that are "harder" to run locally, such as the minified integration tests (see /integration/firestore), but even those are run in CI.

setTimeout(() => {
throw error;
}, 0);
}

// Re-throw the error so that this.tail becomes a rejected Promise and
// all further attempts to chain (via .then) will just short-circuit
// and return the rejected Promise.
Expand Down
15 changes: 0 additions & 15 deletions packages/firestore/src/util/misc.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,25 +56,10 @@ export function primitiveComparator<T>(left: T, right: T): number {
return 0;
}

/** Duck-typed interface for objects that have an isEqual() method. */
export interface Equatable<T> {
isEqual(other: T): boolean;
}

/** Helper to compare nullable (or undefined-able) objects using isEqual(). */
export function equals<T>(
left: Equatable<T> | null | undefined,
right: T | null | undefined
): boolean {
if (left !== null && left !== undefined) {
return !!(right && left.isEqual(right));
} else {
// HACK: Explicitly cast since TypeScript's type narrowing apparently isn't
// smart enough.
return (left as null | undefined) === right;
}
}

/** Helper to compare arrays using isEqual(). */
export function arrayEquals<T>(
left: T[],
Expand Down
14 changes: 1 addition & 13 deletions packages/firestore/src/util/sorted_set.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2017 Google Inc.
* Copyright 2017 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -31,18 +31,6 @@ export class SortedSet<T> {
this.data = new SortedMap<T, boolean>(this.comparator);
}

/**
* Creates a SortedSet from the keys of the map.
* This is currently implemented as an O(n) copy.
*/
static fromMapKeys<K, V>(map: SortedMap<K, V>): SortedSet<K> {
let keys = new SortedSet<K>(map.comparator);
map.forEach(key => {
keys = keys.add(key);
});
return keys;
}

has(elem: T): boolean {
return this.data.get(elem) !== null;
}
Expand Down
18 changes: 1 addition & 17 deletions packages/firestore/test/unit/local/encoded_resource_path.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,7 @@
import { expect } from 'chai';
import {
decodeResourcePath,
encodeResourcePath,
prefixSuccessor
encodeResourcePath
} from '../../../src/local/encoded_resource_path';
import { PersistencePromise } from '../../../src/local/persistence_promise';
import {
Expand Down Expand Up @@ -128,23 +127,8 @@ describe('EncodedResourcePath', () => {
path('food')
]);
});

it('prefixes successors correctly', async () => {
assertPrefixSuccessorEquals('\u0001\u0002', ResourcePath.EMPTY_PATH);
assertPrefixSuccessorEquals(
'foo' + sep + 'bar\u0001\u0002',
path('foo/bar')
);
});
});

function assertPrefixSuccessorEquals(
expected: string,
path: ResourcePath
): void {
expect(prefixSuccessor(encodeResourcePath(path))).to.deep.equal(expected);
}

function assertEncoded(expected: string, path: ResourcePath): Promise<void> {
const encoded = encodeResourcePath(path);
expect(encoded).to.deep.equal(expected);
Expand Down
7 changes: 3 additions & 4 deletions packages/firestore/test/unit/local/lru_garbage_collector.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import { Timestamp } from '../../../src/api/timestamp';
import { User } from '../../../src/auth/user';
import { ListenSequence } from '../../../src/core/listen_sequence';
import { Query } from '../../../src/core/query';
import { SnapshotVersion } from '../../../src/core/snapshot_version';
import { ListenSequenceNumber, TargetId } from '../../../src/core/types';
import { IndexedDbPersistence } from '../../../src/local/indexeddb_persistence';
import {
Expand Down Expand Up @@ -49,7 +48,7 @@ import {
SetMutation
} from '../../../src/model/mutation';
import { AsyncQueue } from '../../../src/util/async_queue';
import { key, path, wrapObject } from '../../util/helpers';
import { key, path, version, wrapObject } from '../../util/helpers';
import { SortedMap } from '../../../src/util/sorted_map';
import * as PersistenceTestHelpers from './persistence_test_helpers';
import { primitiveComparator } from '../../../src/util/misc';
Expand Down Expand Up @@ -246,7 +245,7 @@ function genericLruGarbageCollectorTests(
const key = nextTestDocumentKey();
return new Document(
key,
SnapshotVersion.fromMicroseconds(1000),
version(1000),
wrapObject({
foo: 3,
bar: false
Expand Down Expand Up @@ -784,7 +783,7 @@ function genericLruGarbageCollectorTests(
txn => {
const doc = new Document(
middleDocToUpdate,
SnapshotVersion.fromMicroseconds(2000),
version(2000),
wrapObject({
foo: 4,
bar: true
Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/test/unit/local/reference_set.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2017 Google Inc.
* Copyright 2017 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down
17 changes: 6 additions & 11 deletions packages/firestore/test/unit/local/target_cache.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2017 Google Inc.
* Copyright 2017 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -92,12 +92,7 @@ describe('IndexedDbTargetCache', () => {
originalSequenceNumber
);
const actualTargetData = await targetCache2.getTargetData(target);

if (process.env.USE_MOCK_PERSISTENCE !== 'YES') {
// TODO(b/140573486): This fails on Node with persistence since the
// resume token is read back as a string.
expect(targetData.isEqual(actualTargetData!)).to.be.true;
}
expect(targetData).to.deep.equal(actualTargetData);

const actualSnapshotVersion = await targetCache2.getLastRemoteSnapshotVersion();
expect(snapshotVersion.isEqual(actualSnapshotVersion)).to.be.true;
Expand Down Expand Up @@ -127,12 +122,12 @@ function genericTargetCacheTests(
function testTargetData(
target: Target,
targetId: TargetId,
version?: number
testVersion?: number
): TargetData {
if (version === undefined) {
version = 0;
if (testVersion === undefined) {
testVersion = 0;
}
const snapshotVersion = SnapshotVersion.fromMicroseconds(version);
const snapshotVersion = version(testVersion);
const resumeToken = resumeTokenForSnapshot(snapshotVersion);
return new TargetData(
target,
Expand Down
6 changes: 3 additions & 3 deletions packages/firestore/test/unit/remote/serializer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1420,7 +1420,7 @@ describe('Serializer', () => {
document: {
name: s.toName(key('coll/1')),
fields: wrap({ foo: 'bar' }).mapValue!.fields,
updateTime: s.toVersion(SnapshotVersion.fromMicroseconds(5))
updateTime: s.toVersion(version(5))
},
targetIds: [1, 2]
}
Expand All @@ -1440,7 +1440,7 @@ describe('Serializer', () => {
document: {
name: s.toName(key('coll/1')),
fields: wrap({ foo: 'bar' }).mapValue!.fields,
updateTime: s.toVersion(SnapshotVersion.fromMicroseconds(5))
updateTime: s.toVersion(version(5))
},
targetIds: [2],
removedTargetIds: [1]
Expand All @@ -1459,7 +1459,7 @@ describe('Serializer', () => {
const actual = s.fromWatchChange({
documentDelete: {
document: s.toName(key('coll/1')),
readTime: s.toVersion(SnapshotVersion.fromMicroseconds(5)),
readTime: s.toVersion(version(5)),
removedTargetIds: [1, 2]
}
});
Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/test/unit/util/async_queue.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ describe('AsyncQueue', () => {

it('handles failures', () => {
const queue = new AsyncQueue();
const expected = new Error('Firestore Test Simulated Error');
const expected = new Error('Firit cestore Test Simulated Error');

// Disable logging for this test to avoid the assertion being logged
const oldLogLevel = getLogLevel();
Expand Down
Loading