Skip to content

Remove namespace import for objUtils #2807

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

Remove namespace import for objUtils #2807

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
cd6cf88
Remove namespace import for objUtils
schmidt-sebastian Mar 26, 2020
0778cf7
[AUTOMATED]: License Headers
schmidt-sebastian Mar 26, 2020
401e974
Merge
schmidt-sebastian Mar 27, 2020
a6c63c3
Remove remaining namespace imports
schmidt-sebastian Mar 27, 2020
bccccb5
[AUTOMATED]: License Headers
schmidt-sebastian Mar 27, 2020
b04f425
Try downlevelIteration
schmidt-sebastian Mar 30, 2020
4ebfb3d
Revert "Try downlevelIteration"
schmidt-sebastian Mar 30, 2020
3c9c4d2
Merge
schmidt-sebastian Mar 31, 2020
dead3b5
Add contains
schmidt-sebastian Mar 31, 2020
336c7c7
Review
schmidt-sebastian Mar 31, 2020
47743c2
Lint
schmidt-sebastian Mar 31, 2020
4aeb466
Fix comment
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
28 changes: 10 additions & 18 deletions packages/firestore/src/api/database.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@ import {
} from '../util/input_validation';
import { logError, setLogLevel, LogLevel, getLogLevel } from '../util/log';
import { AutoId } from '../util/misc';
import * as objUtils from '../util/obj';
import { Deferred, Rejecter, Resolver } from '../util/promise';
import { FieldPath as ExternalFieldPath } from './field_path';

Expand Down Expand Up @@ -166,7 +165,7 @@ class FirestoreSettings {
this.host = settings.host;

validateNamedOptionalType('settings', 'boolean', 'ssl', settings.ssl);
this.ssl = objUtils.defaulted(settings.ssl, DEFAULT_SSL);
this.ssl = settings.ssl ?? DEFAULT_SSL;
}
validateOptionNames('settings', settings, [
'host',
Expand Down Expand Up @@ -222,10 +221,8 @@ class FirestoreSettings {
Please audit all existing usages of Date when you enable the new
behavior.`);
}
this.timestampsInSnapshots = objUtils.defaulted(
settings.timestampsInSnapshots,
DEFAULT_TIMESTAMPS_IN_SNAPSHOTS
);
this.timestampsInSnapshots =
settings.timestampsInSnapshots ?? DEFAULT_TIMESTAMPS_IN_SNAPSHOTS;

validateNamedOptionalType(
'settings',
Expand Down Expand Up @@ -341,9 +338,7 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
validateExactNumberOfArgs('Firestore.settings', arguments, 1);
validateArgType('Firestore.settings', 'object', 1, settingsLiteral);

if (
objUtils.contains(settingsLiteral as objUtils.Dict<{}>, 'persistence')
) {
if (settingsLiteral.hasOwnProperty('persistence')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If the user has overridden hasOwnProperty this validation result cannot be trusted. Also, if the object was created with Object.create(null) the object has no prototype and this invocation will throw. Calling Object.prototype.hasOwnProperty.call is the safer way to phrase this. Since we do this in a few places and the reference can't be minified, it's probably cheaper to keep the contains method.

Since this is really validation-related rather than general object manipulation it could make sense to move contains into input_validation.ts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. The only usage that is sensible is in this file, so I moved contains here.

throw new FirestoreError(
Code.INVALID_ARGUMENT,
'"persistence" is now specified with a separate call to ' +
Expand Down Expand Up @@ -398,12 +393,10 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
"firestore.enablePersistence() call to use 'synchronizeTabs'."
);
}
synchronizeTabs = objUtils.defaulted(
settings.synchronizeTabs !== undefined
? settings.synchronizeTabs
: settings.experimentalTabSynchronization,
DEFAULT_SYNCHRONIZE_TABS
);
synchronizeTabs =
settings.synchronizeTabs ??
settings.experimentalTabSynchronization ??
DEFAULT_SYNCHRONIZE_TABS;
}

return this.configureClient(this._persistenceProvider, {
Expand Down Expand Up @@ -558,15 +551,14 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
}

private static databaseIdFromApp(app: FirebaseApp): DatabaseId {
const options = app.options as objUtils.Dict<{}>;
if (!objUtils.contains(options, 'projectId')) {
if (!app.options.hasOwnProperty('projectId')) {
throw new FirestoreError(
Code.INVALID_ARGUMENT,
'"projectId" not provided in firebase.initializeApp.'
);
}

const projectId = options['projectId'];
const projectId = app.options.projectId;
if (!projectId || typeof projectId !== 'string') {
throw new FirestoreError(
Code.INVALID_ARGUMENT,
Expand Down
66 changes: 34 additions & 32 deletions packages/firestore/src/core/sync_engine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ import {
QueryTargetState,
SharedClientStateSyncer
} from '../local/shared_client_state_syncer';
import * as objUtils from '../util/obj';
import { SortedSet } from '../util/sorted_set';
import { ListenSequence } from './listen_sequence';
import { Query, LimitType } from './query';
Expand Down Expand Up @@ -147,13 +146,11 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
private queryViewsByQuery = new ObjectMap<Query, QueryView>(q =>
q.canonicalId()
);
private queriesByTarget: { [targetId: number]: Query[] } = {};
private queriesByTarget = new Map<TargetId, Query[]>();
private limboTargetsByKey = new SortedMap<DocumentKey, TargetId>(
DocumentKey.comparator
);
private limboResolutionsByTarget: {
[targetId: number]: LimboResolution;
} = {};
private limboResolutionsByTarget = new Map<TargetId, LimboResolution>();
private limboDocumentRefs = new ReferenceSet();
/** Stores user completion handlers, indexed by User and BatchId. */
private mutationUserCallbacks = {} as {
Expand Down Expand Up @@ -271,10 +268,11 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {

const data = new QueryView(query, targetId, view);
this.queryViewsByQuery.set(query, data);
if (!this.queriesByTarget[targetId]) {
this.queriesByTarget[targetId] = [];
if (this.queriesByTarget.has(targetId)) {
this.queriesByTarget.get(targetId)!.push(query);
} else {
this.queriesByTarget.set(targetId, [query]);
}
this.queriesByTarget[targetId].push(query);
return viewChange.snapshot!;
}

Expand Down Expand Up @@ -308,10 +306,11 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {

// Only clean up the query view and target if this is the only query mapped
// to the target.
const queries = this.queriesByTarget[queryView.targetId];
const queries = this.queriesByTarget.get(queryView.targetId)!;
if (queries.length > 1) {
this.queriesByTarget[queryView.targetId] = queries.filter(
q => !q.isEqual(query)
this.queriesByTarget.set(
queryView.targetId,
queries.filter(q => !q.isEqual(query))
);
this.queryViewsByQuery.delete(query);
return;
Expand Down Expand Up @@ -399,8 +398,8 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
try {
const changes = await this.localStore.applyRemoteEvent(remoteEvent);
// Update `receivedDocument` as appropriate for any limbo targets.
objUtils.forEach(remoteEvent.targetChanges, (targetId, targetChange) => {
const limboResolution = this.limboResolutionsByTarget[Number(targetId)];
remoteEvent.targetChanges.forEach((targetChange, targetId) => {
const limboResolution = this.limboResolutionsByTarget.get(targetId);
if (limboResolution) {
// Since this is a limbo resolution lookup, it's for a single document
// and it could be added, modified, or removed, but not a combination.
Expand Down Expand Up @@ -479,13 +478,13 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
// PORTING NOTE: Multi-tab only.
this.sharedClientState.updateQueryState(targetId, 'rejected', err);

const limboResolution = this.limboResolutionsByTarget[targetId];
const limboResolution = this.limboResolutionsByTarget.get(targetId);
const limboKey = limboResolution && limboResolution.key;
if (limboKey) {
// Since this query failed, we won't want to manually unlisten to it.
// So go ahead and remove it from bookkeeping.
this.limboTargetsByKey = this.limboTargetsByKey.remove(limboKey);
delete this.limboResolutionsByTarget[targetId];
this.limboResolutionsByTarget.delete(targetId);

// TODO(klimt): We really only should do the following on permission
// denied errors, but we don't have the cause code here.
Expand All @@ -504,7 +503,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
const resolvedLimboDocuments = documentKeySet().add(limboKey);
const event = new RemoteEvent(
SnapshotVersion.MIN,
/* targetChanges= */ {},
/* targetChanges= */ new Map<TargetId, TargetChange>(),
/* targetMismatches= */ new SortedSet<TargetId>(primitiveComparator),
documentUpdates,
resolvedLimboDocuments
Expand Down Expand Up @@ -701,19 +700,19 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
this.sharedClientState.removeLocalQueryTarget(targetId);

assert(
this.queriesByTarget[targetId] &&
this.queriesByTarget[targetId].length !== 0,
this.queriesByTarget.has(targetId) &&
this.queriesByTarget.get(targetId)!.length !== 0,
`There are no queries mapped to target id ${targetId}`
);

for (const query of this.queriesByTarget[targetId]) {
for (const query of this.queriesByTarget.get(targetId)!) {
this.queryViewsByQuery.delete(query);
if (error) {
this.syncEngineListener!.onWatchError(query, error);
}
}

delete this.queriesByTarget[targetId];
this.queriesByTarget.delete(targetId);

if (this.isPrimary) {
const limboKeys = this.limboDocumentRefs.referencesForId(targetId);
Expand All @@ -739,7 +738,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {

this.remoteStore.unlisten(limboTargetId);
this.limboTargetsByKey = this.limboTargetsByKey.remove(key);
delete this.limboResolutionsByTarget[limboTargetId];
this.limboResolutionsByTarget.delete(limboTargetId);
}

private updateTrackedLimbos(
Expand Down Expand Up @@ -772,7 +771,10 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
logDebug(LOG_TAG, 'New document in limbo: ' + key);
const limboTargetId = this.limboTargetIdGenerator.next();
const query = Query.atPath(key.path);
this.limboResolutionsByTarget[limboTargetId] = new LimboResolution(key);
this.limboResolutionsByTarget.set(
limboTargetId,
new LimboResolution(key)
);
this.remoteStore.listen(
new TargetData(
query.toTarget(),
Expand Down Expand Up @@ -823,7 +825,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
})
.then((viewDocChanges: ViewDocumentChanges) => {
const targetChange =
remoteEvent && remoteEvent.targetChanges[queryView.targetId];
remoteEvent && remoteEvent.targetChanges.get(queryView.targetId);
const viewChange = queryView.view.applyChanges(
viewDocChanges,
/* updateLimboDocuments= */ this.isPrimary === true,
Expand Down Expand Up @@ -912,7 +914,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
const activeTargets: TargetId[] = [];

let p = Promise.resolve();
objUtils.forEachNumber(this.queriesByTarget, (targetId, _) => {
this.queriesByTarget.forEach((_, targetId) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could these forEach loops be replaced with simpler and, arguable, more readable,for loops?

e.g.

this.queriesByTarget.forEach((_, targetId) => {

could be changed to:

for (targetId of this.queriesByTarget.keys()) {

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 tried making this work (I pushed two commits and one revert). The problem is that I would have to turn on "downlevelIteration" to make this work with ES5, which adds a lot of code (+4%).

The example you posted actually does not need downlevelIteration since you are converting the Map to an Array. That creates an extra copy though and hence I decided to leave this as is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool. Thanks for looking into this.

if (this.sharedClientState.isLocalQueryTarget(targetId)) {
activeTargets.push(targetId);
} else {
Expand All @@ -936,11 +938,11 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {

// PORTING NOTE: Multi-tab only.
private resetLimboDocuments(): void {
objUtils.forEachNumber(this.limboResolutionsByTarget, targetId => {
this.limboResolutionsByTarget.forEach((_, targetId) => {
this.remoteStore.unlisten(targetId);
});
this.limboDocumentRefs.removeAllReferences();
this.limboResolutionsByTarget = [];
this.limboResolutionsByTarget = new Map<TargetId, LimboResolution>();
this.limboTargetsByKey = new SortedMap<DocumentKey, TargetId>(
DocumentKey.comparator
);
Expand All @@ -959,7 +961,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
const newViewSnapshots: ViewSnapshot[] = [];
for (const targetId of targets) {
let targetData: TargetData;
const queries = this.queriesByTarget[targetId];
const queries = this.queriesByTarget.get(targetId);

if (queries && queries.length !== 0) {
// For queries that have a local View, we need to update their state
Expand Down Expand Up @@ -1051,7 +1053,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
return;
}

if (this.queriesByTarget[targetId]) {
if (this.queriesByTarget.has(targetId)) {
switch (state) {
case 'current':
case 'not-current': {
Expand Down Expand Up @@ -1091,7 +1093,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {

for (const targetId of added) {
assert(
!this.queriesByTarget[targetId],
!this.queriesByTarget.has(targetId),
'Trying to add an already active target'
);
const target = await this.localStore.getTarget(targetId);
Expand All @@ -1108,7 +1110,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
for (const targetId of removed) {
// Check that the target is still active since the target might have been
// removed if it has been rejected by the backend.
if (!this.queriesByTarget[targetId]) {
if (!this.queriesByTarget.has(targetId)) {
continue;
}

Expand Down Expand Up @@ -1138,12 +1140,12 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
}

getRemoteKeysForTarget(targetId: TargetId): DocumentKeySet {
const limboResolution = this.limboResolutionsByTarget[targetId];
const limboResolution = this.limboResolutionsByTarget.get(targetId);
if (limboResolution && limboResolution.receivedDocument) {
return documentKeySet().add(limboResolution.key);
} else {
let keySet = documentKeySet();
const queries = this.queriesByTarget[targetId];
const queries = this.queriesByTarget.get(targetId);
if (!queries) {
return keySet;
}
Expand Down
6 changes: 3 additions & 3 deletions packages/firestore/src/local/encoded_resource_path.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 @@ -72,7 +72,7 @@ const encodedEscape = '\u0011';
/**
* Encodes a resource path into a IndexedDb-compatible string form.
*/
export function encode(path: ResourcePath): EncodedResourcePath {
export function encodeResourcePath(path: ResourcePath): EncodedResourcePath {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is your rationale for renaming these functions? It's not that I disagree, it's just that the motivation is unclear to me.

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 are now used as top-level functions. I renamed them to be more descriptive than just "encode/decode".

Copy link
Contributor

Choose a reason for hiding this comment

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

That's what I figured. Makes sense.

let result = '';
for (let i = 0; i < path.length; i++) {
if (result.length > 0) {
Expand Down Expand Up @@ -114,7 +114,7 @@ function encodeSeparator(result: string): string {
* decoding resource names from the server; those are One Platform format
* strings.
*/
export function decode(path: EncodedResourcePath): ResourcePath {
export function decodeResourcePath(path: EncodedResourcePath): ResourcePath {
// Event the empty path must encode as a path of at least length 2. A path
// with exactly 2 must be the empty path.
const length = path.length;
Expand Down
11 changes: 7 additions & 4 deletions packages/firestore/src/local/indexeddb_index_manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2019 Google Inc.
* Copyright 2019 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,7 +18,10 @@
import { ResourcePath } from '../model/path';
import { assert } from '../util/assert';
import { immediateSuccessor } from '../util/misc';
import { decode, encode } from './encoded_resource_path';
import {
decodeResourcePath,
encodeResourcePath
} from './encoded_resource_path';
import { IndexManager } from './index_manager';
import { IndexedDbPersistence } from './indexeddb_persistence';
import { DbCollectionParent, DbCollectionParentKey } from './indexeddb_schema';
Expand Down Expand Up @@ -64,7 +67,7 @@ export class IndexedDbIndexManager implements IndexManager {

const collectionParent: DbCollectionParent = {
collectionId,
parent: encode(parentPath)
parent: encodeResourcePath(parentPath)
};
return collectionParentsStore(transaction).put(collectionParent);
}
Expand Down Expand Up @@ -93,7 +96,7 @@ export class IndexedDbIndexManager implements IndexManager {
if (entry.collectionId !== collectionId) {
break;
}
parentPaths.push(decode(entry.parent));
parentPaths.push(decodeResourcePath(entry.parent));
}
return parentPaths;
});
Expand Down
Loading