-
Notifications
You must be signed in to change notification settings - Fork 199
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
[gRPC] @typespec/protobuf #636
Conversation
You can try these changes at https://cadlplayground.z22.web.core.windows.net/prs/636/ |
@witemple-msft Can we close this PR? |
Hmm, weird, this happend on only one leg:
|
@nguerrera I just had the pipeline rerun failed steps it and it passed. It could have been some kind of transient issue. |
/** | ||
* Don't emit anything. | ||
*/ | ||
noEmit?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was that not the compilerOptions.noEmit but your own noEmit
option we were talking about in the meeting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a noEmit
option that is part of the options of the protobuf emitter. So it's a second level of noEmit
: one that is at the compiler configuration level, and one that is at the emitter configuration level. I figured that you may want to run the validation for the typespec/protobuf emitter but not actually write any files, for example when developing utility libraries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, this feels like something like emitter-output-dir
we might want to have avaialble for all emitters automatically if this is a real use case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a great sense of whether or not it's a real, common use case yet, and I think I need to see how people will use this emitter in practice before I can fill in those details. The scenario I'd like to support is one where you are developing a library for consumption in TypeSpec that has some Proto-enabled features. You need to run our emitter's validation, but don't want to actually generate any files when you run the compiler because your types are meant to be imported and used by some other library in a Proto context, but maybe there are other contexts where you do want to emit.
export async function $onValidate(program: Program) { | ||
/* c8 ignore next 6 */ | ||
if (program.compilerOptions.noEmit) { | ||
const options = program.emitters.find((e) => e.emitFunction === $onEmit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here should ideally not have to check the emitter options for onValidate. THe code should ideally be able to be validated the same regardless of what is the emitter configuration and then if there is some failure specific to that its probably not worth returning in onValidate.(e.g. FIlename conflict)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've created an issue to track this: #1859. I've also added a link to this issue in the source.
The core problem is that without checking that the noEmit
option is set, I generate double diagnostics, since all diagnostics come from the emitter.
@nguerrera, @timotheeguerin : this "cannot find entrypoint" thing is happening again (only in the Windows CI) and I have no idea what it is trying to tell me. Last time I reran failed steps and it worked, but now that it's continuing to happen I'm thinking I must have configured something wrong. Any ideas? |
cc @allenjzhang since it's coming from typedoc and also looking myself |
I think this is just a build order issue, adding the protobuf as a dep of the website will make sure it gets build ahead |
Nice catch, yeah, that explains why it sometimes works and sometimes doesn't :) |
Curious what steps are left to have this grpc work merged? |
@Petermarcu it's been down to my availability. Working on the last steps now. Just creating issues to track some outstanding work and fixing issues that @timotheeguerin identified. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just those symbol which might cause issue if 2 instance of the lib is loaded.
This PR adds
@typespec/protobuf
, a library for specifying and emitting protobuf files.Overview
This PR has involved months of on/off work, and it's at the point where I really need to merge it to continue. By now it's also very feature complete compared to the original prototype.
Please see the
test/scenarios
folder for several examples of input specs and output Protobuf specs (or expected diagnostics in diagnostics.txt if appropriate).Quick architecture guide:
ast.ts
: A data structure for Protobuf files.proto.ts
: the implementation of decorators.lib.ts
: JS entrypoint and declaration of the TypeSpecProtobufLibrary.trasnform/
: the actual compiler that transforms TypeSpec to the protobuf AST.write.ts
: converters from AST to file strings.There is still some minor refactoring to reduce code duplication to be done, and I need to add documentation to several methods, but overall I consider this ready at this point.
Issues addressed in this branch
Closes all outstanding gRPC issues, as they have all been implemented and tested in the scenarios.
Closes #633
Closes #632
Closes #631
Closes #630
Closes #629
Implementation details for above issues:
Streaming Operations
A decorator
dec TypeSpec.Protobuf.stream(target: Operation, mode: StreamMode);
allows specifying the RPC streaming mode of an operation. One of "Duplex", "In", "Out", and "None", where the default is "None".Multi-package specs
This "just works." If you declare multiple proto packages in your TypeSpec and use messages from one in another, the emitted protobuf files automatically import dependencies as needed.
Integral type encodings
The library exports several scalars that can be used to indicate a specific integer encoding, for example
scalar TypeSpec.Protobuf.sfixed32 extends int32;
, which indicates the fixed-length 32-bit integer encoding in Protobuf.Well-known library types
This is supported through an
Extern
mechanism. A model that extendsExtern<string, string>
refers to a type in an exogenous protobuf file. For example, givenmodel MyExtern extends TypeSpec.Protobuf.Extern<"foo.proto", "Foo">;
, if a message refers toMyExtern
in TypeSpec, the resulting Protobuf will have animport "foo.proto"
statement and the type will be replaced by the literal symbol"Foo"
. Users will have to ensure that other names infoo.proto
don't conflict with anything in the TypeSpec, as we can't check that for them.Integral enums
We support using a TypeSpec
enum
in a message if the TypeSpec enum follows the rules of Protobuf enums:We allow enums to alias values by default and will emit a protobuf option enabling that feature if any aliased entries exist in the enum.
Why
@typespec/protobuf
and not@typespec/grpc
gRPC is an implementation of an RPC system. Protobuf is the specification language. gRPC is by far the most common RPC system used with Protobuf specifications, but it is possible to use other RPC mechanisms. See Google's documentation that explains this: https://developers.google.com/protocol-buffers/docs/proto3#services
What we're emitting are Protocol Buffers specs, not exactly gRPC, so I feel that typespec/protobuf is a more appropriate name.
OUTDATED, ORIGINAL DESCRIPTION BELOW
This is the result of migrating the standalone cadl-grpc package to this repo, linking it to rush, and modifying the code to work with changes to the compiler that have been merged since the gRPC demo.
This PR is still a work in progress. There are several TODOs sprinkled through the code, and each one should either be addressed before this PR is ready or converted into and linked to a tracking issue.
Here's a little architectural walkthrough:
src/
grpc.ts
: Cadl library entrypoint, contains all the decorators and $onEmitlib.ts
: TypeScript entrypoint, contains the Cadl library definition, diagnostics, etc.proto.ts
: an embedded Protobuf DSLtransform.ts
: creates a collection of protobuf files (defined by the DSL) from a Cadl programwrite.ts
: implements writing protobuf DSL nodes to stringsA large open item for me is to take the logic from the emit functions in
transform.ts
that validates the nodes and extract it into functions that can run inside the decorators instead. Then during emit we can just assume that the Cadl tree is valid for Protobuf and avoid revalidating.