Skip to content

Restore Protobuf.Types to GrpcObject tree#448

Closed
dvisztempacct wants to merge 10 commits intogrpc:masterfrom
dvisztempacct:proto-loader-codecs
Closed

Restore Protobuf.Types to GrpcObject tree#448
dvisztempacct wants to merge 10 commits intogrpc:masterfrom
dvisztempacct:proto-loader-codecs

Conversation

@dvisztempacct
Copy link
Copy Markdown

@grpc/proto-loader hides protobufjs' message types from users, making them inaccessible.

This PR suggests a possible approach for making them accessible again.

I'm given to understand that because protobufjs is non-semver and grpc-node would like to remain semver, this has not be done.

I wanted to create this PR as a place to document the details of this decision and explore the possibility of finding some way to do this. I am doubtful this PR will be merged, but hopefully it will accomplish its other goals :)

use instanceof instead of as for distinguishing Services from other
NamespaceBases

collect Types in addition to Services
You can now put protobufjs' message types back where you might have
expected them if you had used grpc with deprecated protobufjs 5 via the
require('grpc').load or similar interface.

Some types from grpc-native-core had to be mocked or redefined here because
I don't know how to typescript.
@thelinuxfoundation
Copy link
Copy Markdown

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

Regards,
CLA GitHub bot

@nicolasnoble
Copy link
Copy Markdown
Contributor

I have a semi-crazy proposition, that I'd need to discuss with @murgatroid99 anyway. But the main reason we decided not to do this was that we didn't want to transitively expose protobufjs' API through, because we can't really control protobufjs' API changes.

How about we add an optional field that is protobufjs directly ? More specifically, something along these lines:

const protobufjs = require('protobufjs')
const protoLoader = require('@grpc/proto-loader')
const loaderOptions = {
  defaults: true,
  protobufjs: protobufjs
}
const servicesAndMessages = protoLoader.loadSync('some.proto', options)

With this proposal, when protobufjs is specified, the loader will use it instead of its own protobufjs dependency, ensuring that the user will know exactly which version of protobufjs is used to expose messages. And when it's specified, proto-loader will then no longer suppress the message types from the results, because this isn't a transitive API exposition anymore.

@dvisztempacct
Copy link
Copy Markdown
Author

@nicolasnoble that sounds like it would work!

@murgatroid99
Copy link
Copy Markdown
Member

I am sorry for the long delay here. If you are still interested in this problem, I have proposed a solution at grpc/proposal#116. Please feel free to comment on it.

@murgatroid99
Copy link
Copy Markdown
Member

Thank you for your contribution. This change has now been superseded by #703.

@lock lock bot locked as resolved and limited conversation to collaborators Apr 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants