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

Stop loading proto files dynamically #2936

Closed
alexander-fenster opened this issue Jun 11, 2019 · 5 comments
Closed

Stop loading proto files dynamically #2936

alexander-fenster opened this issue Jun 11, 2019 · 5 comments
Assignees
Labels
external This issue is blocked on a bug with the actual product. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. web

Comments

@alexander-fenster
Copy link
Contributor

Currently, all gRPC Node.js client libraries load proto files dynamically using @grpc/proto-loader (that internally calls protobufjs.loadSync). Since proto files can form a huge dependency tree, it brings some real time complexity to the client constructor, and (more importantly) it blocks things like webpack (#2933).

There are several possible ways to deal with this dynamic load.

  1. [proposed option] Compile protos to JSON using pbjs -t json running in a postinstall script, and load this JSON file in the runtime. For Node.js, this JSON format can be read by @grpc/proto-loader without making any changes to it (it's an undocumented feature but since protobufjs supports it, @grpc/proto-loader also supports it). For browsers, we'll do protobuf.root.fromJSON(jsonContent) to load the same JSON file which will be required to become a part of the bundle.

  2. Compile proto files to static objects with protoc - this is what other languages do.
    I dislike this option, mostly because the change from protobufjs object to protoc will likely bring some incompatibilities and hard-to-find bugs in the libraries.

  3. Compile proto files to static objects with pbjs. Unfortunately, static objects generated by pbjs -t static-module are not supported by @grpc/grpc-js so we can't use this option for now (even though it's a great option for possible browser usage).

So, from my undestanding at this moment, the first option is the only option that would allow us to make the code working both in Node.js without dynamic proto loading, and in browsers (with no gRPC support). I'm going to work on this option and will update this issue when the code is ready to review.

Cc: @JustinBeckwith @bcoe @guybedford

@alexander-fenster alexander-fenster self-assigned this Jun 11, 2019
@guybedford
Copy link

How important is it to separate the proto files from the main build files in Node.js? Your proposal seems to suggest dynamic JSON loading in Node.js, and a static bundle for the browser. Usually we find the converse - that it is the browser where you want the granular loading of JSON files, and in Node.js where it is better to have everything in one file for their performance.

@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Jun 12, 2019
@alexander-fenster
Copy link
Contributor Author

@guybedford So the problem here with the underlying library (gRPC) that for now cannot consume the statically created proto object. So we'll try to see how JSON protos performs in both cases (both Node and browser). I can write more words but let me better share some code that will show the idea :) (later this week)

@alexander-fenster alexander-fenster added priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed triage me I really want to be triaged. labels Jun 12, 2019
@yoshi-automation yoshi-automation added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. labels Jun 17, 2019
@yoshi-automation yoshi-automation added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. labels Jun 27, 2019
@yoshi-automation yoshi-automation added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. labels Jul 7, 2019
@yoshi-automation yoshi-automation removed the 🚨 This issue needs some love. label Jul 17, 2019
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Jul 29, 2019
@bcoe bcoe added the external This issue is blocked on a bug with the actual product. label Aug 1, 2019
@yoshi-automation yoshi-automation removed the 🚨 This issue needs some love. label Aug 1, 2019
@timconnorz
Copy link

Any update here? I'm having issues with webpack & google cloud node client

@JustinBeckwith
Copy link
Contributor

Greetings! If you're having issues, please file a new issue letting us know specifically what you're trying, and what problems you're running into :) I don't think this is the issue you're looking for :P

@alexander-fenster
Copy link
Contributor Author

@timconnorz Just an update, we've been loading protos from a pre-compiled JSON for a year already, and we have a {fallback: true} option (that also works in Node.js, not just in browsers) that completely removes fs module from the equation. This issue is pretty much outdated so I'm going to close it - feel free to open an issue for the library you're trying to use in its corresponding repository (e.g. googleapis/nodejs-speech, googleapis/nodejs-vision, etc.)

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external This issue is blocked on a bug with the actual product. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. web
Projects
None yet
Development

No branches or pull requests

6 participants