Remove Cyclic Dependencies #4160

schmidt-sebastian · 2020-12-03T22:25:16Z

This PR should unblock the Console build and should also allow us to revive our old goal to sort our imports.

It also adds a build rule change to block us from introducing circular dependencies in the future (ec5ac56)

wilhuff

Very impressive!

One high level concern I have with the PR as is, is that it's not necessarily obvious where to look for the implementation of a given type. For example, in some cases it's the _impl but in others it's _methods. Also, whether or not this split even exists seems arbitrary.

It also seems that if you squint just right, this is a very similar change to the forward declarations change we made in iOS C++: declare the exported types separately from the actual implementations to drastically reduce the number of files imported by those type declarations. In the iOS implementation, we created a single file that "exported" the types for a package, e.g. model in [model_fwd.h](https://github.com/firebase/firebase-ios-sdk/blob/master/Firestore/core/src/model/model_fwd.h). I wonder if a similar approach here could make this feel more regular.

wilhuff · 2020-12-04T19:56:48Z

packages/firestore/src/model/object_value.ts

+ *             If foo is not an object, foo is replaced with an object
+ *             containing foo
+ */
+export class FieldMask {


FieldMask is in a separate file on other platforms. In some cases (e.g database.ts) I know circularity is impossible to avoid and so we have to dump them all in a single file, but elsewhere keeping classes separate and matching structure across platforms is helpful.

Done. Works without any adjustments.

wilhuff · 2020-12-04T20:00:41Z

packages/firestore/src/model/type_order.ts

+ * limitations under the License.
+ */
+
+export const enum TypeOrder {


Now that this is in a separate file, maybe give it a comment?

https://github.com/firebase/firebase-ios-sdk/blob/master/Firestore/core/src/model/field_value.h#L62 would be suitable.

Done. I actually added a small comment in a later commit, but the one you provided is better.

wilhuff · 2020-12-04T20:03:55Z

packages/firestore/src/core/target.ts

@@ -196,3 +194,436 @@ export function isDocumentTarget(target: Target): boolean {
    target.filters.length === 0
  );
 }
+
+export abstract class Filter {


Now that everything pertaining to queries isn't in a single file, it seems like we could split this further to make it more like the other platforms. While "target" exists as a class there, it doesn't seem like the obvious home for everything else.

Perhaps create:

filter.ts for Filter, FieldFilter, the various Filter implementations, and canonify/stringify ops

order_by.ts for Direction, OrderBy, and canonify/stringify ops

bound.ts for Bound, sortsBeforeDocument, and others

?

Will address in follow up.

wilhuff · 2020-12-04T20:26:30Z

packages/firestore/src/remote/value_serializer.ts

@@ -0,0 +1,61 @@
+/**


"value serializer" seems like it could include anything we might think to put in the value proto. Would it make sense to move things like Timestamp, bytes, and GeoPoint serialization into here too?

This also matches the trend on the other platforms to split value proto handling into a separate file too.

The serialization of these types actually happens in UserDataReader. The Bytes/Timestamp methods in serializer are only used as helpers for the Watch protos (among other things).

I renamed the file to number_serializer for now.

wilhuff · 2020-12-04T20:28:49Z

packages/firestore/src/local/persistence_transaction.ts

@@ -0,0 +1,40 @@
+/**


PersistenceTransactionMode seems like something you might look for here too.

Edit: it seems like this was addressed in a later commit.

wilhuff · 2020-12-04T21:23:12Z

packages/firestore/src/api/field_value.ts

+ * Sentinel values that can be used when writing document fields with `set()`
+ * or `update()`.
+ */
+export abstract class FieldValue {


I can't say I really understand the layering model we have here and why this move is desirable.

It seems like conceptually the classic API should only ever point to the lite API and not the other way around. Allowing the other order encourages cycles and the possibility of polluting the lite implementation with classes that can't be tree shaken.

Sorry, this is also something that was fixed in a later commit. It is indeed in lite/src/api/field_value

wilhuff · 2020-12-04T21:25:57Z

packages/firestore/src/api/user_data_reader.ts

@@ -398,6 +405,148 @@ export function parseSetData(
  );
 }

+export class DeleteFieldValueImpl extends FieldValue {


A possible thought to follow up on layering: would it make sense to explicitly break out common code and the classic API as separate paths? That way you could impose layering checks, e.g.

lite -> common

exp -> (lite, common)

classic -> (exp, common)

or similar?

Once all the dust has settled, I would like to move the /exp and /lite files under /src. They only contain API code and the nested imports are not pretty. This is something I will consider then.

wilhuff · 2020-12-04T21:27:46Z

packages/firestore/lite/src/api/components.ts

 // The components module manages the lifetime of dependencies of the Firestore
 // client. Dependencies can be lazily constructed and only one exists per
 // Firestore instance.

+export interface FirestoreService extends _FirebaseService {


Comments, please. In particular what's a FirestoreService and how does it differ from a FirebaseFirestore instance?

I added the following:

/** * An interface implemented by FirebaseFirestore that provides * compatibility with the usage in this file. * * This interface mainly exists to remove a cyclic dependency. */

I will explore doing the same FirebaseFirestore/FirebaseFirestoreImpl pattern in a follow-up PR.

packages/firestore/exp/src/api/reference.ts

wilhuff · 2020-12-04T21:31:14Z

packages/firestore/lite/src/api/query.ts

+  options: SetOptions
+): Promise<void>;
+
+export function setDoc<T>(


Setting, updating, and adding documents doesn't seem to have anything to do with "queries". Did you mean to move this here given that reference.ts still exists?

Alternatively, if this can't exist in reference.ts to solve circularity issues, maybe create reference_impl.ts to match the structure you've established elsewhere (e.g. with LocalStoreImpl)?

Sorry, this was also cleaned up in a later commit, which I know is impossible to detect with a PR this size.

I did rename the file to reference_impl.ts as well.

schmidt-sebastian

Feedback addressed (or punted to follow up work for next week). Sorry about this PR and thanks for the review!

schmidt-sebastian · 2020-12-04T23:32:06Z

packages/firestore/src/core/target.ts

@@ -196,3 +194,436 @@ export function isDocumentTarget(target: Target): boolean {
    target.filters.length === 0
  );
 }
+
+export abstract class Filter {


Will address in follow up.

schmidt-sebastian · 2020-12-04T23:32:30Z

packages/firestore/src/model/type_order.ts

+ * limitations under the License.
+ */
+
+export const enum TypeOrder {


Done. I actually added a small comment in a later commit, but the one you provided is better.

schmidt-sebastian · 2020-12-04T23:32:46Z

packages/firestore/src/model/object_value.ts

+ *             If foo is not an object, foo is replaced with an object
+ *             containing foo
+ */
+export class FieldMask {


Done. Works without any adjustments.

schmidt-sebastian · 2020-12-04T23:34:19Z

packages/firestore/src/remote/value_serializer.ts

@@ -0,0 +1,61 @@
+/**


The serialization of these types actually happens in UserDataReader. The Bytes/Timestamp methods in serializer are only used as helpers for the Watch protos (among other things).

I renamed the file to number_serializer for now.

schmidt-sebastian · 2020-12-04T23:35:34Z

packages/firestore/src/local/persistence_transaction.ts

@@ -0,0 +1,40 @@
+/**


schmidt-sebastian · 2020-12-05T01:16:41Z

packages/firestore/src/model/path.ts

@@ -366,3 +366,57 @@ export class FieldPath extends BasePath<FieldPath> {
    return new FieldPath([]);
  }
 }
+
+export class DocumentKey {


I swear it did when I looked at it, but it seems to have magically disappeared. Reverted this change.

schmidt-sebastian · 2020-12-05T01:18:03Z

packages/firestore/src/api/field_value.ts

+ * Sentinel values that can be used when writing document fields with `set()`
+ * or `update()`.
+ */
+export abstract class FieldValue {


Sorry, this is also something that was fixed in a later commit. It is indeed in lite/src/api/field_value

schmidt-sebastian · 2020-12-05T01:19:30Z

packages/firestore/src/api/user_data_reader.ts

@@ -398,6 +405,148 @@ export function parseSetData(
  );
 }

+export class DeleteFieldValueImpl extends FieldValue {


Once all the dust has settled, I would like to move the /exp and /lite files under /src. They only contain API code and the nested imports are not pretty. This is something I will consider then.

schmidt-sebastian · 2020-12-05T01:23:05Z

packages/firestore/lite/src/api/components.ts

 // The components module manages the lifetime of dependencies of the Firestore
 // client. Dependencies can be lazily constructed and only one exists per
 // Firestore instance.

+export interface FirestoreService extends _FirebaseService {


I added the following:

/** * An interface implemented by FirebaseFirestore that provides * compatibility with the usage in this file. * * This interface mainly exists to remove a cyclic dependency. */

I will explore doing the same FirebaseFirestore/FirebaseFirestoreImpl pattern in a follow-up PR.

schmidt-sebastian · 2020-12-05T01:24:26Z

packages/firestore/lite/src/api/query.ts

+  options: SetOptions
+): Promise<void>;
+
+export function setDoc<T>(


Sorry, this was also cleaned up in a later commit, which I know is impossible to detect with a PR this size.

I did rename the file to reference_impl.ts as well.

schmidt-sebastian · 2020-12-05T01:31:17Z

package/packages/firebase/firebase-messaging.js

@@ -2560,14 +2560,12 @@
                  );
                case 1:
                  return (
-                    e


FYI: This is removed in a commit later.

schmidt-sebastian · 2020-12-05T01:32:14Z

packages/firestore/exp/src/api/database.ts

@@ -68,7 +69,7 @@ export const CACHE_SIZE_UNLIMITED = LRU_COLLECTION_DISABLED;
 * Do not call this constructor directly. Instead, use {@link getFirestore}.
 */
 export class FirebaseFirestore extends LiteFirestore {
-  readonly _queue = newAsyncQueue();
+  readonly _queue: AsyncQueue = newAsyncQueue();


Somehow the AsyncQueue type is required to not break the APIExtractor toolchain.

wilhuff

LGTM

wilhuff · 2020-12-07T23:24:24Z

packages/firestore/src/model/document_key.ts

@@ -1,6 +1,6 @@
 /**
 * @license
- * Copyright 2017 Google LLC
+ * Copyright 2020 Google LLC


If you change this back to 2017 and add a newline back after import { debugAssert } this file we be unchanged in this PR.

wilhuff · 2020-12-07T23:25:30Z

packages/firestore/src/remote/backoff.ts

@@ -1,6 +1,6 @@
 /**
 * @license
- * Copyright 2017 Google LLC
+ * Copyright 2020 Google LLC


If you change this back to 2017 and revert the import reordering, this file could now be unchanged in this PR.

schmidt-sebastian added 30 commits December 3, 2020 09:41

Move FieldMask

5599705

Move normalizeTimestamp

b8efc13

Move TypeOrder

6af13ee

Move query

f315c89

Move toDouble, toInteger

bf8f746

Move Serializer

fb16170

Move PersistenceTransaction

b682857

Move ExponentialBackoff

6e32f92

Move LocalStoreImpl

e2846a9

LruGarbageCollector

8fcc47b

Move PRIMARY_LEASE_LOST_ERROR_MSG

3456d83

BundleReader

0fba935

Move BundleLoader

3e69755

IndexedDbBundleCache

a4758a0

IndexedDbIndexManager

1b1a67d

Clean up IndexedDbPersistence

11df697

Remove unused imports

1159302

removeMutationBatch/DocumentKey

e5b9a81

IndexedDbSchemaConverter

b639a29

IndexedDbBundleCache

a503100

MemoryTargetCache

cccabf2

Move FieldValue

0b518f0

Move FieldValue implementations

cb07140

Fix Components

d9c66ec

Make Reference unique

c06dd59

Move ensureClientConfigured/configureClient

d03efd3

Move SnapshotMetadata

c2f951e

Fix lite/.../query.ts

2c0faa2

Move UserDataWriter

97b2189

Add onwarn

ec5ac56

Merge

a1f8f77

wilhuff reviewed Dec 4, 2020

View reviewed changes

schmidt-sebastian assigned schmidt-sebastian and unassigned wilhuff Dec 4, 2020

schmidt-sebastian added 10 commits December 4, 2020 16:16

Add separate FieldMask

e66d39d

Add TypeOrder comment

57ff562

s/numberSerializer/valueSerializer

a4001fc

AsyncQueue/AsyncQueueImpl

618017d

s/_methods/_impl

a35b1d6

getStore

a8e3409

BATCHID_UNKNOWN

0294aa3

removeMutationBatch

6482dae

DocumentKey

b15d996

FirestoreService

045f33f

schmidt-sebastian commented Dec 5, 2020

View reviewed changes

Fix bad merge

55fb014

schmidt-sebastian commented Dec 5, 2020

View reviewed changes

schmidt-sebastian changed the title ~~Remove Circular Dependencies~~ Remove Cyclic Dependencies Dec 5, 2020

schmidt-sebastian assigned wilhuff and unassigned schmidt-sebastian Dec 5, 2020

wilhuff approved these changes Dec 7, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Dec 7, 2020

schmidt-sebastian added 4 commits December 7, 2020 17:05

Feedback

d41071c

Merge

9f9cfaf

Fix bundles

25d414c

Undo API Extractor

d180871

schmidt-sebastian merged commit 2541375 into master Dec 8, 2020

firebase locked and limited conversation to collaborators Jan 8, 2021

Remove Cyclic Dependencies #4160

Remove Cyclic Dependencies #4160

Uh oh!

Conversation

schmidt-sebastian commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilhuff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilhuff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

schmidt-sebastian commented Dec 3, 2020 •

edited

Loading