Untangle Datastore #2971

schmidt-sebastian · 2020-04-22T18:14:58Z

This removes the Datastore dependencies on SortedMap, AsyncQueue and the persistent streams, all of which are not needed in the "Rest Wrapper" client.

google-oss-bot · 2020-04-22T18:23:36Z

Binary Size Report

Affected SDKs

@firebase/firestore

Type Base (d92d67b) Head (bc3cf36) Diff

browser 247 kB 247 kB +302 B (+0.1%)

esm2017 193 kB 193 kB +96 B (+0.0%)

main 486 kB 487 kB +1.10 kB (+0.2%)

module 245 kB 245 kB +248 B (+0.1%)
@firebase/firestore/memory

Type Base (d92d67b) Head (bc3cf36) Diff

browser 188 kB 188 kB +302 B (+0.2%)

esm2017 148 kB 148 kB +96 B (+0.1%)

main 363 kB 364 kB +1.10 kB (+0.3%)

module 186 kB 186 kB +248 B (+0.1%)

`firebase`

Type	Base (`d92d67b`)	Head (`bc3cf36`)	Diff
firebase-firestore.js	289 kB	289 kB	+200 B (+0.1%)
firebase-firestore.memory.js	231 kB	231 kB	+200 B (+0.1%)
firebase.js	822 kB	822 kB	+205 B (+0.0%)

Test Logs

Base (d92d67b2): https://github.com/firebase/firebase-js-sdk/actions/runs/86996371
Head (bc3cf36a): https://github.com/firebase/firebase-js-sdk/actions/runs/87157729

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

packages/firestore/src/remote/datastore.ts

    private connection: Connection,
    private credentials: CredentialsProvider,
    private serializer: JsonProtoSerializer
  ) {}

-  newPersistentWriteStream(


Rather than changing the object models and completely rearranging the client (so that it doesn't match the other ports), a better strategy would be to adopt tree-shakeable APIs internally.

That is, make classes like Datastore more like just dumb structs, and make the methods that used to be in the class plain functions that take Datastore as an argument.

This will greatly reduce the amount of churn in the relative arrangement of components. Indeed, you could apply this to commit and other methods on datastore to ensure that all of it tree-shakes out if unused.

One of the downsides is that it makes every member public, and I kind of wanted to avoid this. Your argument that we should pull out all methods does win here though. I updated the PR, which also greatly reduces the diff (a large part are now just test cleanup).

That's what I meant by dumb structs. Tree-shakeable APIs make this code look like old-school C.

Is it possible to adopt a convention of which members are actually public vs those which would be private, but are public by virtue of this kind of code structure?

In C programs a way to handle this was to forward declare a private member of your struct to hide details:

typedef struct DatastoreDetails DatastoreDetails; typedef struct Datastore { DatastoreDetails* details; } Datastore;

Then you only define struct DatastoreDetails in your .c file.

A similar technique could be translated into typescript:

The exported type contains a private details: unknown

DatastoreDetails is not exported

The constructor assigns an instance of DatastoreDetails to the details field.

There's a non-exported details(datastore: Datastore) => DatastoreDetails that casts

I'm fairly certain this isn't actually worth it, but this is how we implemented access control before it existed in the language.

I pushed a commit with a TypeScript version of this hack: 0295cd5

It should be pretty easy to understand, but I wanted to point out a downside: With this change, at least IntelliJ now consider all members of DatastoreImpl unused, which would breaks its automated refactor should we use it on classes like this.

That's a lot better than my suggestion. If we can get IntelliJ happy, I think this is the way to go.

wilhuff · 2020-04-22T21:24:26Z

packages/firestore/test/integration/remote/stream.test.ts

-      writeStream.start();
-      return streamListener.awaitCallback('open');
-    }).then(async () => {
+    return withTestWriteStream(async (writeStream, streamListener) => {


Despite the other comment on thinking the newPersistentStream methods should stay in Datastore, this change is a good one (though withTestWriteStream should call newPersistentWriteStream(datastore).

wilhuff · 2020-04-23T17:26:22Z

packages/firestore/src/remote/datastore.ts

    private connection: Connection,
    private credentials: CredentialsProvider,
    private serializer: JsonProtoSerializer
  ) {}

-  newPersistentWriteStream(


That's what I meant by dumb structs. Tree-shakeable APIs make this code look like old-school C.

Is it possible to adopt a convention of which members are actually public vs those which would be private, but are public by virtue of this kind of code structure?

In C programs a way to handle this was to forward declare a private member of your struct to hide details:

typedef struct DatastoreDetails DatastoreDetails; typedef struct Datastore { DatastoreDetails* details; } Datastore;

Then you only define struct DatastoreDetails in your .c file.

A similar technique could be translated into typescript:

The exported type contains a private details: unknown

DatastoreDetails is not exported

The constructor assigns an instance of DatastoreDetails to the details field.

There's a non-exported details(datastore: Datastore) => DatastoreDetails that casts

I'm fairly certain this isn't actually worth it, but this is how we implemented access control before it existed in the language.

wilhuff · 2020-04-23T17:40:21Z

packages/firestore/src/remote/datastore.ts

+    database: datastore.serializer.encodedDatabaseId,
+    writes: mutations.map(m => datastore.serializer.toMutation(m))
+  };
+  return datastore


Might as well make this async/await.

wilhuff · 2020-04-23T17:44:29Z

packages/firestore/src/remote/datastore.ts

+    writes: mutations.map(m => datastore.serializer.toMutation(m))
+  };
+  return datastore
+    .invokeRPC<CommitRequest, api.CommitResponse>('Commit', params)


The stylistic mix of functions and methods seems arbitrary. It seems like the Datastore "class" could just completely convert.

Optional.

I'd rather keep this as is, but we can revisit if this pattern becomes a problem.

wilhuff · 2020-04-23T17:56:46Z

packages/firestore/src/remote/datastore.ts

+  return datastore
+    .invokeStreamingRPC<
+      BatchGetDocumentsRequest,
+      api.BatchGetDocumentsResponse


Why is it BatchGetDocumentsRequest but api.BatchGetDocumentsResponse?

BatchGetDocumentsRequest is api. BatchGetDocumentsRequest with the database field set. I just added it in the Proto d.ts field since I/we have been editing this file manually lately.

wilhuff · 2020-04-23T20:33:12Z

packages/firestore/test/integration/remote/remote.test.ts

-          return ds.lookup([k]);
-        })
-        .then((docs: MaybeDocument[]) => {
+      return invokeCommitRpc(ds, [mutation])


async/await?

Converted this file

wilhuff · 2020-04-24T18:39:44Z

packages/firestore/src/remote/datastore.ts

+ * Cloud Datastore grpc API, which provides an interface that is more convenient
+ * for the rest of the client SDK architecture to consume.
+ */
+export class Datastore {}


If you made this an interface this wouldn't even materialize in the generated code.

The downside is that empty interfaces match any object so we'd effectively lose type safety. Maybe that's why you chose class?

Yes, an empty interface matches all objects in TypeScript. It is even commonly used to do exactly that.

wilhuff · 2020-04-24T18:44:58Z

packages/firestore/src/remote/datastore.ts

+  datastore: Datastore,
+  mutations: Mutation[]
+): Promise<MutationResult[]> {
+  debugAssert(


Is IntelliJ happier if you made this something like this here:

const datastoreImpl = cast(datastore);

and also added a function like this:

function cast(datastore: Datastore): DatastoreImpl { debugAssert(...); return datastore as DatastoreImpl; }

I just used an explicit cast to a new type, which makes IntelliJ happy.

A cast function also has the benefit of reducing the boilerplate at the top of every function from 5 lines to 1. The specific text of which function needed DatastoreImpl isn't important to preserve--should we run afoul of this assertion the stack will show with function was invoked improperly.

If you want to avoid the extra function in the emitted code (if that's a problem) we could add a rewrite rule to inline the function call (to just a cast). If cast is too ambiguous for the strategy, maybe name it castDatastoreImpl, recoverDatastoreImpl, 'toDatastoreImpl, or asDatastoreImpl`? This way you could write dumb, unscoped global rules for this (again--if that's even worthwhile).

Not 100% convinced that this effort was worth it (since all these extra lines are stripped in production), but I added a debugCast function that not only taught me a lot about TypeScript and constructor signatures but also can be used everywhere else in the code.

Agreed that this makes little to no difference in the production code (after stripping). Where it makes a difference is in the readability of this required transformation. Having the same 5-line incantation at the top of nearly every function would have made my eyes bleed :-).

wilhuff · 2020-04-24T18:45:23Z

packages/firestore/src/remote/datastore.ts

    private connection: Connection,
    private credentials: CredentialsProvider,
    private serializer: JsonProtoSerializer
  ) {}

-  newPersistentWriteStream(


That's a lot better than my suggestion. If we can get IntelliJ happy, I think this is the way to go.

wilhuff

Basically LGTM, except I think we should find a way to cut down the 5 line prolog to every function definition in here now.

wilhuff · 2020-04-24T19:52:44Z

packages/firestore/src/remote/datastore.ts

+  datastore: Datastore,
+  mutations: Mutation[]
+): Promise<MutationResult[]> {
+  debugAssert(


A cast function also has the benefit of reducing the boilerplate at the top of every function from 5 lines to 1. The specific text of which function needed DatastoreImpl isn't important to preserve--should we run afoul of this assertion the stack will show with function was invoked improperly.

If you want to avoid the extra function in the emitted code (if that's a problem) we could add a rewrite rule to inline the function call (to just a cast). If cast is too ambiguous for the strategy, maybe name it castDatastoreImpl, recoverDatastoreImpl, 'toDatastoreImpl, or asDatastoreImpl`? This way you could write dumb, unscoped global rules for this (again--if that's even worthwhile).

wilhuff

LGTM

wilhuff · 2020-04-24T20:43:58Z

packages/firestore/src/remote/datastore.ts

+  datastore: Datastore,
+  mutations: Mutation[]
+): Promise<MutationResult[]> {
+  debugAssert(


Agreed that this makes little to no difference in the production code (after stripping). Where it makes a difference is in the readability of this required transformation. Having the same 5-line incantation at the top of nearly every function would have made my eyes bleed :-).

Untangle Datastore

dbb548e

schmidt-sebastian force-pushed the mrschmidt/reads branch from 1128453 to dbb548e Compare April 22, 2020 18:22

schmidt-sebastian requested a review from wilhuff April 22, 2020 18:26

schmidt-sebastian added the api: firestore label Apr 22, 2020

schmidt-sebastian assigned wilhuff Apr 22, 2020

Lint

18425c1

wilhuff reviewed Apr 22, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Apr 22, 2020

Review

c7eef78

schmidt-sebastian assigned wilhuff and unassigned schmidt-sebastian Apr 23, 2020

wilhuff reviewed Apr 23, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Apr 23, 2020

schmidt-sebastian added 4 commits April 23, 2020 16:18

Hide members

0295cd5

Async/Await/Fix Types

c26d02a

Convert test

b91cb9f

createDatastore -> newDatastore to match

cc64f76

schmidt-sebastian assigned wilhuff and unassigned schmidt-sebastian Apr 23, 2020

Merge

5543422

wilhuff reviewed Apr 24, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Apr 24, 2020

Add cast

92f5142

schmidt-sebastian assigned wilhuff and unassigned schmidt-sebastian Apr 24, 2020

wilhuff reviewed Apr 24, 2020

View reviewed changes

schmidt-sebastian added 3 commits April 24, 2020 13:23

Cast

104602c

Remove only

7e17920

Fix test

e6c0780

wilhuff approved these changes Apr 24, 2020

View reviewed changes

Update misc.test.ts

be47948

schmidt-sebastian merged commit 8c36958 into master Apr 24, 2020

scottcrossen pushed a commit that referenced this pull request May 4, 2020

Untangle Datastore (#2971)

3b9f7eb

firebase locked and limited conversation to collaborators May 25, 2020

schmidt-sebastian deleted the mrschmidt/reads branch July 9, 2020 17:45

Untangle Datastore #2971

Untangle Datastore #2971

Uh oh!

Conversation

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

Uh oh!

Uh oh!

google-oss-bot commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Report

Affected SDKs

@firebase/firestore

@firebase/firestore/memory

firebase

Test Logs

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!

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!

wilhuff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

schmidt-sebastian commented Apr 22, 2020 •

edited

Loading

google-oss-bot commented Apr 22, 2020 •

edited

Loading

`@firebase/firestore`

`@firebase/firestore/memory`

`firebase`