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

fix: change style of comment in protos for protobufjs #150

Closed
wants to merge 2 commits into from

Conversation

alexander-fenster
Copy link
Contributor

So this one is weird.


TL;DR: in this PR we convert all // comments to /** ... */ in all the proto files that are copied to the generated client library.


In the monolith generator, we created the whole src/v*/doc folder (e.g. here) that was pretty much the copy of protos with comments, converted to JavaScript for jsdoc.

In the micro-generator, we don't do that. Instead, we run pbjs to generate protos/protos.js and then let jsdoc read that file, which just works.

But, I just realized that jsdoc does not consider // style comments from proto files and does not copy them to the resulting protos/protos.js. Its documentation says here:

ProTip! Documenting your .proto files with /** ... */-blocks or (trailing) /// ... lines translates to generated static code.

So... we must convert the proto comments from // to /** ... */ to make them go into the jsdoc. Luckily, we have a great place to do that - right in the code where we copy proto files into the resulting directory. This PR does exactly that.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 20, 2019
@JustinBeckwith
Copy link
Contributor

Yeah, this is weird 😆 Should we be proposing a change in jsdoc or protobufjs instead? I'm a little worried about trying to accurately parse JavaScript in a way that's complete/effective here.

@alexander-fenster
Copy link
Contributor Author

I'm only parsing protos, not JavaScript, so it's probably easier and more predictable.

jsdoc is out of question, it just reads what is in protos/protos.js. But protobufjs did not put the comments there because they ignore //-style comments.

Fixing this in pbjs may be an option, but a harder one (let me say this - I spent like half an hour staring at that code and did not immediately figure out how to make it work there). So I still opt for local proto rewrite.

Note that we don't really use these protos for anything in the runtime (they are now published just for reference, we don't load them), and if any parsing problem happens, we'll catch it when synthtool fails (when we run npx compileProtos src).

@alexander-fenster
Copy link
Contributor Author

I need to think more about the proper way of doing this, given that we now can make protobuf.js releases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants