Stop using WebChannelConnection in Lite SDK #3482

schmidt-sebastian · 2020-07-24T23:52:02Z

This PR changes the Lite SDK to use the native networking stack in favor of WebChannel. It uses fetch on Browsers, and node-fetch on Node (which is API compatible with fetch).

Most of the code in this PR is to plumb new platform settings through (which I am not happy about, but I wasn't able to come up with another approach). The main problem is that the Node Lite SDK needs to use Proto3-Json if we switch away from WebChannel, which means that we need to change all callsites for newSerializer. To make this even worse, I added a test-only environment variable to allow me to dynamically import the correct platform. Suggestions welcome.

google-oss-bot · 2020-07-25T00:03:31Z

Binary Size Report

Affected SDKs

`@firebase/firestore`

Type	Base (`2fa0353`)	Head (`4ca5ca1`)	Diff
browser	247 kB	247 kB	+307 B (+0.1%)
esm2017	193 kB	193 kB	+202 B (+0.1%)
main	472 kB	472 kB	+262 B (+0.1%)
module	244 kB	245 kB	+283 B (+0.1%)
react-native	193 kB	194 kB	+190 B (+0.1%)

`@firebase/firestore/exp`

Type	Base (`2fa0353`)	Head (`4ca5ca1`)	Diff
browser	187 kB	187 kB	+202 B (+0.1%)
main	464 kB	465 kB	+262 B (+0.1%)
module	187 kB	187 kB	+202 B (+0.1%)
react-native	187 kB	187 kB	+190 B (+0.1%)

`@firebase/firestore/lite`

Type	Base (`2fa0353`)	Head (`4ca5ca1`)	Diff
browser	67.9 kB	63.3 kB	-4.58 kB (-6.8%)
main	144 kB	141 kB	-3.08 kB (-2.1%)
module	67.9 kB	63.3 kB	-4.58 kB (-6.8%)
react-native	67.9 kB	63.4 kB	-4.54 kB (-6.7%)

`@firebase/firestore/memory`

Type	Base (`2fa0353`)	Head (`4ca5ca1`)	Diff
browser	185 kB	186 kB	+289 B (+0.2%)
esm2017	145 kB	145 kB	+206 B (+0.1%)
main	347 kB	347 kB	+262 B (+0.1%)
module	183 kB	184 kB	+291 B (+0.2%)
react-native	145 kB	145 kB	+192 B (+0.1%)

`firebase`

Type	Base (`2fa0353`)	Head (`4ca5ca1`)	Diff
firebase-firestore.js	286 kB	286 kB	+254 B (+0.1%)
firebase-firestore.memory.js	226 kB	226 kB	+254 B (+0.1%)
firebase.js	819 kB	819 kB	+255 B (+0.0%)

Test Logs

Base (2fa0353d): https://github.com/firebase/firebase-js-sdk/actions/runs/193764826
Head (4ca5ca11): https://github.com/firebase/firebase-js-sdk/actions/runs/194008047

schmidt-sebastian · 2020-07-25T01:03:12Z

Looks like I need to plumb the JSON serialization settings through to all places where we do serialization. Back to drawing board.

changeset-bot · 2020-07-25T03:24:27Z

💥 No Changeset

Latest commit: c38f0fa

Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂

If these changes should be published to npm, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Feiyang1 · 2020-07-31T16:48:04Z

packages/firestore/src/platform/node_lite/http_connection.ts

+ * limitations under the License.
+ */
+
+import axios, { AxiosRequestConfig, AxiosResponse } from 'axios';


How about using a fetch polyfill e.g. isomorphic-fetch, so browser and node can share implementation? So this file can be removed and node_lite/connection.ts becomes:

import `isomorphic-fetch`; export * from '../browser_lite/connection';

I like the idea. I ended up using node-fetch, because:

isomorphic-fetch is not maintained anymore

node-fetch is not a Polyfill. I think it is better if we don't inject our own code into the global namspace.

Feiyang1 · 2020-07-31T16:57:55Z

packages/firestore/src/platform/connection.ts

-    return node.newConnection(databaseInfo);
+    return isLite()
+      ? nodeLite.newConnection(databaseInfo)
+      : node.newConnection(databaseInfo);
  } else if (isReactNative()) {
    return rn.newConnection(databaseInfo);


Can we add the isLite() check for RN for completeness? I know we won't hit this code path currently. and similarly in platform/serializer.ts.

I actually reworked this code. We would have to change every callsite and it gets quite ugly with all those branches. Instead, I am now using an environment variable to set the platform for ts-node. It feels a little less ugly to me, but opinions can differ here...

Feiyang1 · 2020-07-31T17:01:02Z

packages/firestore/src/platform/browser_lite/fetch_connection.ts

+      );
+    }
+
+    return JSON.parse(await response.text());


Is it different than just await response.json()?

Nope. Shortened.

Feiyang1 · 2020-07-31T17:10:55Z

packages/firestore/test/integration/util/events_accumulator.ts

@@ -16,7 +16,7 @@
 */

 import * as firestore from '@firebase/firestore-types';
-import { Deferred } from '@firebase/util';
+import { Deferred } from '../../util/promise';


Can we just use Deferred from @firebase/util?

This is the only usage of the Deferred promise in Firestore. Since I realized this is test-only, it probably doesn't matter much. I can revert this change with my next update to this PR (I just got the CI to green after one week...)

Removed this change.

Feiyang1 · 2020-07-31T17:24:31Z

packages/firestore/test/unit/remote/rest_connection.test.ts

+    try {
+      await connection.invokeRPC('RunQuery', {}, null);
+      fail();
+    } catch (e) {
+      expect(e.code).to.equal(Code.UNKNOWN);
+    }


Does this work?

expect(connection.invokeRPC('RunQuery', {}, null)).to.be.eventually.rejectedWith(Error).and.have.property('code', Code.UNKNOWN);

Updated (with a simplification)

Feiyang1 · 2020-08-03T20:52:12Z

packages/firestore/test/integration/bootstrap.ts

@@ -24,6 +24,8 @@ import '../../index';
 * https://github.com/webpack-contrib/karma-webpack#alternative-usage
 */

+process.env.TEST_PLATFORM = 'browser';


Why do you need to set it if if it is the browser test? TEST_PLATFORM is only used for node test, right?

Yes... I had to change the webpack bundler to allow me to inject browser-lite. With that change, we are now using the new networking stack for the Lite test. The change in this file was meant to do this, but was 100% wrong (but with the best of intentions!).

wilhuff · 2020-08-03T20:43:04Z

packages/firestore/src/remote/rest_connection.ts

+      urlRpcName !== undefined,
+      'Unknown REST mapping for: ' + rpcName
+    );
+    const path = ((req as unknown) as Indexable).parent || this.databaseRoot;


This seems like it could be more self-contained if the caller passed the path they wanted based on the RPC they were invoking. As it stands this is kind of magical because Indexable doesn't say anything about how the type should work.

I changed the callsites to pass in the path and also removed the manual deletion of parent and database.

wilhuff · 2020-08-03T20:47:58Z

packages/firestore/src/remote/rpc_error.ts

@@ -243,9 +248,9 @@ export function mapCodeFromHttpStatus(status: number): Code {
      return Code.OK;

    case 400: // Bad Request
-      return Code.INVALID_ARGUMENT;
+      return Code.FAILED_PRECONDITION;


This doesn't seem right. A 400 status most typically indicates that the client has sent a bad request, not that the request is valid but can't be performed now. INVALID_ARGUMENT is the primary status in https://cloud.google.com/apis/design/errors#handling_errors.

So we currently send this request to write a non-existing document in a transaction:

writes: [ { update: { name: 'projects/test-emulator/databases/(default)/documents/test-collection/noHB7gQj6Tp98raxkpxU', fields: { counter: { integerValue: '1' } } }, currentDocument: { updateTime: '1970-01-01T00:00:00.000000000Z' } } ]

This updateTime: epoch trick/hack seems to do the same thing as { currentDocument: { exists: false }} but it returns a 400 (against Prod and the emulator). If we return INVALID_ARGUMENT here, the transaction is not retried.

One thing to note is that mapCodeFromHttpStatus was actually never used before, so at least this change should not affect any existing callsites.

wilhuff · 2020-08-03T20:56:10Z

packages/firestore/src/remote/rest_connection.ts

+ * Base class for all Rest-based connections to the backend (WebChannel and
+ * HTTP).
+ */
+export abstract class RestConnection implements Connection {


Within any given build isn't the case that there will ever be one connection type? It seems like the bundler could pick which "instance" you get and we could make these all free functions. Or is that coming in a separate PR?

We can do this either way, but I thought it would be cleaner to use inheritance here. There will only ever be one implementation for this class in any given bundle.

wilhuff

LGTM

schmidt-sebastian requested review from Feiyang1, hiranya911 and hsubox76 as code owners July 24, 2020 23:52

schmidt-sebastian assigned wilhuff and Feiyang1 Jul 24, 2020

schmidt-sebastian added the api: firestore label Jul 24, 2020

schmidt-sebastian unassigned Feiyang1 and wilhuff Jul 25, 2020

schmidt-sebastian force-pushed the mrschmidt/rest branch 4 times, most recently from 8e04faa to 46c46df Compare July 31, 2020 03:05

Remove WebChannelConnection from Lite SDK

e63bea7

schmidt-sebastian force-pushed the mrschmidt/rest branch from 46c46df to e63bea7 Compare July 31, 2020 03:15

CI

2894484

Feiyang1 reviewed Jul 31, 2020

View reviewed changes

schmidt-sebastian added 8 commits July 31, 2020 10:52

next run

f7aaab9

Using node-fetch

fd30f0c

WIP

c02e623

Lint

3ea3327

Merge

918aacb

Cleanup

6b37f2c

--amend

198e11d

Undo yarn update

3c8b6f5

schmidt-sebastian assigned wilhuff and Feiyang1 Jul 31, 2020

Feiyang1 reviewed Aug 3, 2020

View reviewed changes

Feiyang1 approved these changes Aug 3, 2020

View reviewed changes

wilhuff reviewed Aug 3, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Aug 3, 2020

schmidt-sebastian force-pushed the mrschmidt/rest branch from 7869a3e to 135b557 Compare August 3, 2020 23:36

schmidt-sebastian assigned wilhuff Aug 3, 2020

Review

c38f0fa

schmidt-sebastian force-pushed the mrschmidt/rest branch from 135b557 to c38f0fa Compare August 3, 2020 23:38

wilhuff approved these changes Aug 4, 2020

View reviewed changes

wilhuff removed their assignment Aug 4, 2020

schmidt-sebastian merged commit 7f9b3d9 into master Aug 4, 2020

schmidt-sebastian deleted the mrschmidt/rest branch August 4, 2020 21:21

firebase locked and limited conversation to collaborators Sep 4, 2020

Stop using WebChannelConnection in Lite SDK #3482

Stop using WebChannelConnection in Lite SDK #3482

Uh oh!

Conversation

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

Uh oh!

Uh oh!

google-oss-bot commented Jul 25, 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/exp

@firebase/firestore/lite

@firebase/firestore/memory

firebase

Test Logs

Uh oh!

schmidt-sebastian commented Jul 25, 2020

Uh oh!

changeset-bot bot commented Jul 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💥 No Changeset

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!

Uh oh!

schmidt-sebastian commented Jul 24, 2020 •

edited

Loading

google-oss-bot commented Jul 25, 2020 •

edited

Loading

`@firebase/firestore`

`@firebase/firestore/exp`

`@firebase/firestore/lite`

`@firebase/firestore/memory`

`firebase`

changeset-bot bot commented Jul 25, 2020 •

edited

Loading