Revive Node.JS Support for Cloud Firestore (fixes #221). #319

mikelehen · 2017-11-16T01:50:33Z

Fix miscellaneous node / GRPC bit rot.
- Re-add grpc / protobufjs dependencies.
- Remove crazy grpc stream error handling nonsense that is no longer
  necessary after Make event order consistent, and make 'end' and 'error' mutually exclusive grpc/grpc#9101
- Clean up grpc_connection logging (and consistently use util.inspect()).
Fix WebChannel / GRPC Connection compatibility issues.
- Add an explicit mapping from "RPC name" (e.g. "BatchGetDocuments") to the
  REST url path (e.g. "batchGet") for WebChannel, and for GRPC just assume
  the first letter should be lowercased (e.g. "batchGetDocuments").
- Split Connection.invoke() into invokeRPC() and invokeStreamingRPC(), with
  the latter accepting a stream of results and aggregating them into an
  array (needed to support BatchGetDocuments RPC).
Fix serializer issues
- Query limits are an 'Int32Value' but we were serializing them as a normal
  int which GRPC / protobufjs didn't like.
- Several proto "oneof tags" were outdated.
Add build steps to copy protos into npm package.
Run integration tests for Node.JS
- Added to 'test:node' script in package.json and in .vscode/launch.json
- Include index.ts for browser and index.node.ts for node so the appropriate
  PlatformSupport gets registered.
Misc cleanup
- Remove unused MockPlatform since we use the normal NodePlatform now.
- Remove 'google-auth-library' CredentialsProvider that we used to use for
  node.js (before we were integrated with FirebaseAuth).
- Fixed several tests that were hitting node.js warnings about unhandled
  promise failures.
- mocha commmand-line args:
  - "--compilers ts:ts-node/register" was deprecated in favor of
    "--require ts-node/register"
  - Remove "--retries 5" when running mocha tests from VS Code.
  - Consistently use "--require" instead of "-r"
  - Add "--exit" when running from VS Code.

* Fix miscellaneous node / GRPC bit rot. * Re-add grpc / protobufjs dependencies. * Remove crazy grpc stream error handling nonsense that is no longer necessary after grpc/grpc#9101 * Clean up grpc_connection logging (and consistently use util.inspect()). * Fix WebChannel / GRPC Connection compatibility issues. * Add an explicit mapping from "RPC name" (e.g. "BatchGetDocuments") to the REST url path (e.g. "batchGet") for WebChannel, and for GRPC just assume the first letter should be lowercased (e.g. "batchGetDocuments"). * Split Connection.invoke() into invokeRPC() and invokeStreamingRPC(), with the latter accepting a stream of results and aggregating them into an array (needed to support BatchGetDocuments RPC). * Fix serializer issues * Query limits are an 'Int32Value' but we were serializing them as a normal int which GRPC / protobufjs didn't like. * Several proto "oneof tags" were outdated. * Add build steps to copy protos into npm package. * Run integration tests for Node.JS * Added to 'test:node' script in package.json and in .vscode/launch.json * Include index.ts for browser and index.node.ts for node so the appropriate PlatformSupport gets registered. * Misc cleanup * Remove unused MockPlatform since we use the normal NodePlatform now. * Remove 'google-auth-library' CredentialsProvider that we used to use for node.js (before we were integrated with FirebaseAuth). * Fixed several tests that were hitting node.js warnings about unhandled promise failures. * mocha commmand-line args: * "--compilers ts:ts-node/register" was deprecated in favor of "--require ts-node/register" * Remove "--retries 5" when running mocha tests from VS Code. * Consistently use "--require" instead of "-r" * Add "--exit" when running from VS Code.

schmidt-sebastian

:) I'm so happy.

Supposedly WebStorm EAP will allow us to run tests from the IDE. I'm still running into errors, but determined to make this work once this is merged.

schmidt-sebastian · 2017-11-16T19:24:05Z

packages/firestore/package.json

@@ -18,7 +18,9 @@
  "module": "dist/esm/index.js",
  "license": "Apache-2.0",
  "dependencies": {
-    "@firebase/webchannel-wrapper": "^0.2.4"
+    "@firebase/webchannel-wrapper": "^0.2.4",
+    "grpc": "^1.6.6",


Did you try to run these tests with the latest versions of these dependencies?

I have now... It turns out grpc now has typings defined so we could remove our hacky ones (I also had to tweak a few things to make our code compatible with the defined typings).

schmidt-sebastian · 2017-11-16T19:26:23Z

packages/firestore/src/platform_browser/webchannel_connection.ts

@@ -339,6 +333,8 @@ export class WebChannelConnection implements Connection {

  // visible for testing
  makeUrl(rpcName: string): string {
+    const urlRpcName = RPC_NAME_REST_MAPPING[rpcName];
+    assert(urlRpcName != null, 'Unknown REST mapping for: ' + rpcName);


This should be !== undefined.

It can be !== undefined but I tend to not differentiate "undefined" and "null" when I don't need to, so I did the more inclusive != null check. But I also don't care in the slightest... So switched. :-)

schmidt-sebastian · 2017-11-16T19:30:46Z

packages/firestore/src/platform_node/grpc_connection.ts

+
+    // RPC Methods have the first character lower-cased
+    // (e.g. Listen => listen(), BatchGetDocuments => batchGetDocuments()).
+    const rpcMethod = rpcName.charAt(0).toLowerCase() + rpcName.slice(1);


Can we use the same manually-provided mapping that we use in webchannel?

No... the mapping is different because the REST endpoints are manually defined (you can see them in firestore.proto if you want). So e.g. the BatchGetDocuments RPC shows up as .batchGetDocuments() in GRPC but as .../batchGet in the REST URL.

Sorry, I wasn't more clear. I meant the mapping technique (a hardcoded map instead of this on-the-fly conversion).

I like the on-the-fly conversion since it means we don't have to hardcode all of the RPCs we use. With WebChannel we can't avoid the hardcoding since the mapping isn't deterministic (whereas I /think/ it is for GRPC).

schmidt-sebastian · 2017-11-16T19:41:58Z

packages/firestore/src/platform_node/grpc_connection.ts

-      // trigger the 'finish' event).
-      remoteEnded = true;
-      grpcStream.end();
+      close();


This now calls the onClose callback, whereas before it didn't. This seems like a requirement to let the Write/Watch stream know that it ended, but I wanted to make sure this is intentional.

Yeah. We used to call grpcStream.end() which would eventually call 'finish' which would call close() [it was convoluted to work around GRPC error handling bugs]. Now we do it directly.

schmidt-sebastian · 2017-11-16T19:48:12Z

packages/firestore/src/platform_node/grpc_connection.ts

+      );
+    });
+
+    grpcStream.on('status', (status: GrpcStatus) => {
      if (!closed) {


This is obsolete now, isn't it?

Woops, good catch. Yes. I meant to remove this. (done)

schmidt-sebastian · 2017-11-16T19:50:04Z

packages/firestore/src/remote/connection.ts

+    rpcName: string,
+    request: any,
+    token: Token | null
+  ): Promise<any>;


Can this be of type any[]?

Sure can! (done)

schmidt-sebastian · 2017-11-16T21:35:13Z

packages/firestore/test/integration/browser/webchannel.test.ts

@@ -20,6 +20,11 @@ import { DatabaseId, DatabaseInfo } from '../../../src/core/database_info';
 import * as utilHelpers from '../util/helpers';

 describe('WebChannel', () => {


Can we do something like: (typedef window | describe ? xdescribe)('WebChannel', ...

Apparently (according to https://mochajs.org/), the correct way to skip tests programmatically is:

before(function() { if (/* check test environment */) { // setup code } else { this.skip(); } });

But I tried that and it was kinda' crappy (you have to use function() { ... } instead of () => { ... } for this to work, and it only skips the top-level tests, not the nested describe() tests.... So I did it your way. :-)

…ings.

mikelehen

Thanks! PTAL.

mikelehen · 2017-11-17T21:35:56Z

packages/firestore/package.json

@@ -18,7 +18,9 @@
  "module": "dist/esm/index.js",
  "license": "Apache-2.0",
  "dependencies": {
-    "@firebase/webchannel-wrapper": "^0.2.4"
+    "@firebase/webchannel-wrapper": "^0.2.4",
+    "grpc": "^1.6.6",


I have now... It turns out grpc now has typings defined so we could remove our hacky ones (I also had to tweak a few things to make our code compatible with the defined typings).

mikelehen · 2017-11-17T21:38:03Z

packages/firestore/src/platform_browser/webchannel_connection.ts

@@ -339,6 +333,8 @@ export class WebChannelConnection implements Connection {

  // visible for testing
  makeUrl(rpcName: string): string {
+    const urlRpcName = RPC_NAME_REST_MAPPING[rpcName];
+    assert(urlRpcName != null, 'Unknown REST mapping for: ' + rpcName);


It can be !== undefined but I tend to not differentiate "undefined" and "null" when I don't need to, so I did the more inclusive != null check. But I also don't care in the slightest... So switched. :-)

mikelehen · 2017-11-17T21:40:03Z

packages/firestore/src/platform_node/grpc_connection.ts

+
+    // RPC Methods have the first character lower-cased
+    // (e.g. Listen => listen(), BatchGetDocuments => batchGetDocuments()).
+    const rpcMethod = rpcName.charAt(0).toLowerCase() + rpcName.slice(1);


No... the mapping is different because the REST endpoints are manually defined (you can see them in firestore.proto if you want). So e.g. the BatchGetDocuments RPC shows up as .batchGetDocuments() in GRPC but as .../batchGet in the REST URL.

mikelehen · 2017-11-17T21:41:13Z

packages/firestore/src/platform_node/grpc_connection.ts

-      // trigger the 'finish' event).
-      remoteEnded = true;
-      grpcStream.end();
+      close();


Yeah. We used to call grpcStream.end() which would eventually call 'finish' which would call close() [it was convoluted to work around GRPC error handling bugs]. Now we do it directly.

mikelehen · 2017-11-17T21:41:41Z

packages/firestore/src/platform_node/grpc_connection.ts

+      );
+    });
+
+    grpcStream.on('status', (status: GrpcStatus) => {
      if (!closed) {


Woops, good catch. Yes. I meant to remove this. (done)

mikelehen · 2017-11-17T21:43:00Z

packages/firestore/src/remote/connection.ts

+    rpcName: string,
+    request: any,
+    token: Token | null
+  ): Promise<any>;


Sure can! (done)

mikelehen · 2017-11-17T21:49:12Z

packages/firestore/test/integration/browser/webchannel.test.ts

@@ -20,6 +20,11 @@ import { DatabaseId, DatabaseInfo } from '../../../src/core/database_info';
 import * as utilHelpers from '../util/helpers';

 describe('WebChannel', () => {


Apparently (according to https://mochajs.org/), the correct way to skip tests programmatically is:

before(function() { if (/* check test environment */) { // setup code } else { this.skip(); } });

But I tried that and it was kinda' crappy (you have to use function() { ... } instead of () => { ... } for this to work, and it only skips the top-level tests, not the nested describe() tests.... So I did it your way. :-)

schmidt-sebastian · 2017-11-18T00:07:22Z

packages/firestore/src/platform_node/load_protos.ts

-): ProtobufProtoBuilder | undefined {
-  const builder = protobufjs.newBuilder({
+export function loadProtos(): grpc.GrpcObject {
+  const options = {
    // Beware that converting fields to camel case does not convert the tag
    // fields in oneof groups (!!!).


Does this refer to valueType? ProtobufJS 6.x+ is now doing this.

Interesting. I tried to go down the route of depending directly on protobufjs again and upgrading to 6.x, but it did not seem straightforward (the entire way we're consuming protobufjs has changed an doesn't seem to be documented in the release notes, etc.). So I'm punting on this for now. I did update the comment to mention that upgrading to protobufjs 6.x would resolve this though.

schmidt-sebastian

Please look at the comment in grpc_connection.ts

mikelehen

Thanks!

mikelehen · 2017-11-20T19:20:54Z

packages/firestore/src/platform_node/grpc_connection.ts

+
+    // RPC Methods have the first character lower-cased
+    // (e.g. Listen => listen(), BatchGetDocuments => batchGetDocuments()).
+    const rpcMethod = rpcName.charAt(0).toLowerCase() + rpcName.slice(1);


I like the on-the-fly conversion since it means we don't have to hardcode all of the RPCs we use. With WebChannel we can't avoid the hardcoding since the mapping isn't deterministic (whereas I /think/ it is for GRPC).

mikelehen · 2017-11-20T19:23:06Z

packages/firestore/src/platform_node/load_protos.ts

-): ProtobufProtoBuilder | undefined {
-  const builder = protobufjs.newBuilder({
+export function loadProtos(): grpc.GrpcObject {
+  const options = {
    // Beware that converting fields to camel case does not convert the tag
    // fields in oneof groups (!!!).


Interesting. I tried to go down the route of depending directly on protobufjs again and upgrading to 6.x, but it did not seem straightforward (the entire way we're consuming protobufjs has changed an doesn't seem to be documented in the release notes, etc.). So I'm punting on this for now. I did update the comment to mention that upgrading to protobufjs 6.x would resolve this though.

kylehotchkiss · 2017-11-27T17:36:12Z

Hello! When will this be launched in the firebase package on NPM?

mikelehen · 2017-11-27T19:28:10Z

Likely this Thursday. You can keep an eye on https://firebase.google.com/support/release-notes/js to see when it goes out.

jmensch1 · 2017-11-30T23:21:14Z

hey thanks for the update! I think there's a problem though. I upgraded to 4.7.0 with npm, and now when I initialize firestore I'm getting the error 'firebase.firestore is not a function'

My imports are the same they were before:

import * as firebase from 'firebase'
import 'firebase/firestore'

And I initialize firestore by calling firebase.firestore()

jshcrowthe · 2017-11-30T23:56:08Z

@jmensch1 feel free to open an issue with more context (hopefully a sample repro) of the behavior you are seeing. I was unable to reproduce (just verified across a node build and a browser build).

mikelehen added the api: firestore label Nov 16, 2017

mikelehen assigned wilhuff Nov 16, 2017

mikelehen requested review from jshcrowthe, schmidt-sebastian and wilhuff as code owners November 16, 2017 01:50

google-oss-bot added the needs-triage label Nov 16, 2017

mikelehen added enhancement and removed needs-triage labels Nov 16, 2017

mikelehen assigned schmidt-sebastian and unassigned wilhuff Nov 16, 2017

jshcrowthe mentioned this pull request Nov 16, 2017

Firestore fails to load protobuf file #303

Closed

schmidt-sebastian suggested changes Nov 16, 2017

View reviewed changes

schmidt-sebastian assigned mikelehen and unassigned schmidt-sebastian Nov 16, 2017

Michael Lehenbauer added 4 commits November 16, 2017 17:19

upgrade grpc

71e547c

Remove protobufjs dependency (load protos via GRPC) and deal with typ…

5dd5a6f

…ings.

Merge branch 'master' into mikelehen-node-support

da40c3d

Code review tweaks.

5da1096

mikelehen commented Nov 17, 2017

View reviewed changes

Merge branch 'master' into mikelehen-node-support

18d5f0b

mikelehen assigned schmidt-sebastian and unassigned mikelehen Nov 17, 2017

schmidt-sebastian reviewed Nov 18, 2017

View reviewed changes

schmidt-sebastian approved these changes Nov 18, 2017

View reviewed changes

schmidt-sebastian assigned mikelehen and unassigned schmidt-sebastian Nov 18, 2017

Add note about upgrading to protobufjs 6.x fixing casing in oneof tags.

32fa319

mikelehen commented Nov 20, 2017

View reviewed changes

jshcrowthe approved these changes Nov 20, 2017

View reviewed changes

mikelehen merged commit 087375c into master Nov 20, 2017

mikelehen deleted the mikelehen-node-support branch November 20, 2017 19:37

firebase locked and limited conversation to collaborators Oct 24, 2019

		@@ -20,6 +20,11 @@ import { DatabaseId, DatabaseInfo } from '../../../src/core/database_info';
		import * as utilHelpers from '../util/helpers';

		describe('WebChannel', () => {

Revive Node.JS Support for Cloud Firestore (fixes #221). #319

Revive Node.JS Support for Cloud Firestore (fixes #221). #319

Conversation

mikelehen commented Nov 16, 2017

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikelehen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian left a comment

Choose a reason for hiding this comment

mikelehen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylehotchkiss commented Nov 27, 2017

mikelehen commented Nov 27, 2017

jmensch1 commented Nov 30, 2017

jshcrowthe commented Nov 30, 2017 • edited Loading

schmidt-sebastian Nov 16, 2017 •

edited

Loading

jshcrowthe commented Nov 30, 2017 •

edited

Loading