Skip to content

Switch serializer tests to test round trips + remove deprecated grpc.load() #1699

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.

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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions packages/firestore/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
"@firebase/logger": "0.1.11",
"@firebase/webchannel-wrapper": "0.2.17",
"grpc": "1.19.0",
"@grpc/proto-loader": "^0.4.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to run yarn or yarn install from the root and check in the resulting yarn.lock file.

"tslib": "1.9.3"
},
"peerDependencies": {
Expand Down
26 changes: 15 additions & 11 deletions packages/firestore/src/platform_node/load_protos.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
* limitations under the License.
*/

import * as protoLoader from '@grpc/proto-loader';
import * as grpc from 'grpc';
import { resolve } from 'path';

Expand All @@ -24,19 +25,22 @@ import { resolve } from 'path';
* @returns The GrpcObject representing our protos.
*/
export function loadProtos(): grpc.GrpcObject {
const options = {
// Beware that converting fields to camel case does not convert the tag
// fields in oneof groups (!!!). This will likely be fixed when we upgrade
// to protobufjs 6.x
convertFieldsToCamelCase: true
};
const root = resolve(
__dirname,
process.env.FIRESTORE_PROTO_ROOT || '../protos'
);
const firestoreProtoFile = {
root,
file: 'google/firestore/v1/firestore.proto'
};
return grpc.load(firestoreProtoFile, /*format=*/ 'proto', options);
const firestoreProtoFile = root + '/google/firestore/v1/firestore.proto';

// Beware that converting fields to camel case does not convert the tag fields
// in oneof groups (!!!). This will likely be fixed when we upgrade to
// protobufjs 6.x
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment is probably stale since:

  1. gprc/proto-loader pulls in protobufjs 6.8.6.
  2. This comment used to be referring to our usage of 'convertFieldsToCamelCase' which no longer exists.

I think this comment is referring to the fact that the tag fields are named value_type, response_type, etc. instead of valueType, responseType, etc. And I guess this is not going to change (since you didn't have to change that code in serializer.ts), so I would just drop the comment.

const packageDefinition = protoLoader.loadSync(firestoreProtoFile, {
longs: String,
enums: String,
defaults: true,
oneofs: true,
includeDirs: [root]
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it's worth commenting these options a bit so somebody (future me) doesn't have to go look up the grpc docs to figure out what they mean...


return grpc.loadPackageDefinition(packageDefinition);
}
Loading