Skip to content
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

Rework library to be compatible with grpc-js #67

Merged
merged 6 commits into from
Apr 3, 2020

Conversation

murgatroid99
Copy link
Collaborator

This modifies the library so that it exports a single function that takes as an argument an implementation of gRPC and returns the objects, functions, and classes that the library currently exports.

I used grpc-js library for the types but not any of the runtime objects, so the compiled code does not reference it. I also removed references to 'protobufjs' because it wasn't used anywhere.

The only changes I made to the tests were to make them test against both implementations of grpc in a loop, and to use Server#bindAsync instead of Server#bind because that's what both libraries support.

I used TypeScript's export = syntax so that the library could be used like this: const grpcGcp = require('grpc-gcp')(grpcImplementation); to get the same This is a pattern I've seen elsewhere that seemed to fit here, but I'm not sure if it's the best way to do this.

A note to keep in mind: if a user tries to use this library with the original grpc library, and try to preserve the strongest TypeScript typechecks everywhere, they're going to run into some unavoidable problems. There are some sort-of-internal interfaces that don't match between the libraries, but need to be part of the public API. The upside is that this can be worked around with a simple grpc as (typeof grpcJS), and the direct users of grpc-gcp will probably have to deal with the same problem anyway if they allow their users to set the grpc implementation.

@murgatroid99
Copy link
Collaborator Author

I ran the unit tests on my local machine but the integration tests threw credentials errors that seemed to be unrelated to my changes.

@murgatroid99
Copy link
Collaborator Author

I figured out how to run the integration tests, and they don't currently pass.

@murgatroid99
Copy link
Collaborator Author

I had to make a few significant changes to the integration tests to make them work, but I think they still all test the same functionality:

  • I updated to a newer version of google-gax so that we can set what grpc implementation it uses.
  • I updated to a newer version of grpc-tools and changed the code generation to generate package definition objects, which can be loaded into both libraries.
  • I changed some of the API tests in the local service integration tests to more closely match the intended usage of those APIs.

Copy link
Member

@WeiranFang WeiranFang left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes! Just a few questions mentioned in the comments.

Also, do you mind update the readme as well to align with the changes?

grpc-gcp/package.json Outdated Show resolved Hide resolved
callOptions: grpc.CallOptions;
callback?: Function;
}
type GrpcModule = typeof grpcType;
Copy link
Member

Choose a reason for hiding this comment

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

looks like this GrpcModule is also defined in grpc_channel_factory, can we reuse that one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This type is directly derived from an imported type. It should be treated like an imported type, so it doesn't make sense to re-export it.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense.

@@ -1,6 +1,6 @@
{
"name": "grpc-gcp",
"version": "0.2.0",
"version": "0.3.0",
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest we make it as 1.0.0 as a major change, WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For 0.x versions, the standard is to treat the minor version as the major version. In other words, 0.n to 0.n+1 is treated like a major version bump.

With that being said, I wouldn't be opposed to making it 1.0.0, if you are happy with using that version number for this version of the library.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see. So if some library is using ^0.2.0 for grpc-gcp, then this 0.3.0 won't be picked automatically, correct? In that case, I think we are good to use 0.3.0.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is correct.

@murgatroid99
Copy link
Collaborator Author

I made the requested changes to the README.

@WeiranFang WeiranFang merged commit 8542f7e into GoogleCloudPlatform:master Apr 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants