-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat: support parsing of complex options #1744
feat: support parsing of complex options #1744
Conversation
* test: adding test for pbjs static code generation * fix: fromObject should not initialize oneof members
This option skips generation of service clients. Co-authored-by: Alexander Fenster <[email protected]>
* deps: set @types/node to star version When using `protobuf.js` as a dependency in a project it is important that `@types/node` package gets de-duped and has the same version as for the rest of the modules in the project. Otherwise, typing conflicts could happen as they do between v13 and v14 node types. * fix: use @types/node >=13.7.0 * fix: use @types/node >=13.7.0 Co-authored-by: Alexander Fenster <[email protected]> Co-authored-by: Alexander Fenster <[email protected]>
Co-authored-by: xiaoweili <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@alexander-fenster Will be glad if you could take a look at this |
+1 |
In the meanwhile this fix is published as https://www.npmjs.com/package/@cldcvr/protobufjs because I needed it to make https://www.npmjs.com/package/@cldcvr/protobufjs-typescript-gen |
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.
Thank you! This will go into v7.0 that we are going to publish next week.
@NikhilVerma Would you please fix linting, or grant the contributors write access to your branch? Thank you! |
@alexander-fenster Thank you! I've added you as the collaborator to my repo |
@alexander-fenster I've fixed the lint issues thanks! |
Hi @NikhilVerma, I switched your PR to be against Also, the CI should be green now so we are good to go after some extra 👀 . Thank you! |
Actually, the tests now fail with
do you have an idea why is that? I can also take a look but it will take me longer to figure out. |
@alexander-fenster While checking I realised I had written some unnecessary code, it already handled arrays just didn't handle them in all conditions. So I've simplified the code and the tests pass now with the simpler code. |
@alexander-fenster Amazing! thanks for the collaboration :) Looking forward to v7 |
I'm not sure the issue was fixed though. I have the following Protobuf: message AwesomeMessage {
option (option.extension) = {
scopes: [SCOPE_PRODUCT]
visibility: VISIBILITY_PUBLIC
};
repeated Label labels = 1;
LabelSource labels_source = 2 [(option.field).visibility=VISIBILITY_INTERNAL];
} and decoding still fails with error |
@emarkk Are you using v7.0.0? I just tried this on v7 and it worked fine syntax = "proto3";
message AwesomeMessage {
option (option.extension) = {
scopes: [SCOPE_PRODUCT]
visibility: VISIBILITY_PUBLIC
};
repeated Label labels = 1;
LabelSource labels_source = 2 [(option.field).visibility=VISIBILITY_INTERNAL];
}
enum LabelSource {
LABEL_SOURCE_UNKNOWN = 0;
LABEL_SOURCE_USER = 1;
LABEL_SOURCE_AUTO = 2;
}
enum Label {
LABEL_UNKNOWN = 0;
LABEL_USER = 1;
LABEL_AUTO = 2;
} |
Ok, my bad. It was just a cache issue. Works fine with v7. Thanks a lot! |
This allows parsing of complex options and fixes #1743