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.

Sign up for GitHub

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

Jump to bottom

Fix nullability of non-optional fields in TS interfaces and class-level JS comments #1780

Closed
wants to merge 7 commits into from
Closed

Fix nullability of non-optional fields in TS interfaces and class-level JS comments #1780

wants to merge 7 commits into from

Conversation

martin-traverse
Copy link
Contributor

No description provided.

@martin-traverse martin-traverse mentioned this pull request Jul 27, 2022
Closed
@martin-traverse
Copy link
Contributor Author

Relevant issue is #1779

@martin-traverse martin-traverse force-pushed the fix/typescript_optional_support branch 3 times, most recently from 6486157 to a823006 Compare October 18, 2022 08:34
@mamiksik
Copy link

Hey, would you have ETA for when this will be merged please?

@andrea11
Copy link

Hello, any chance to see this merged?

@martin-traverse martin-traverse force-pushed the fix/typescript_optional_support branch from a823006 to 971c974 Compare May 26, 2023 09:40
@martin-traverse
Copy link
Contributor Author

Hello - I have rebased this PR on top of the latest master. Also as per #1779, the change is hidden behind a new flag --pb3-optional. If the flag is not specified, the old behaviour is retained.

@alexander-fenster - are you happy to merge with these changes? Let me know if there is anything else you think would be helpful. We're coming up to the next release series of our product, so for us it would be great to have this in time for that release.

Asondo
Asondo reviewed Jan 9, 2024
View reviewed changes
cli/pbjs.js Outdated
@@ -148,6 +149,7 @@ exports.main = function main(args, callback) {
" --force-message Enforces the use of message instances instead of plain objects.",
"",
" --null-defaults Default value for optional fields is null instead of zero value.",
" --pb3-optional Make type declarations respect optional fields for PB3.",
Copy link

@Asondo Asondo Jan 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be just "--disable-defaults" and thats all? There is no data in grpc package on default value, so, why we should use some magic default or unexpected null value instead of nothing? It is undefined and thats all. So, just disable defaults and it'll be ok)

@zargold
Copy link

zargold commented May 8, 2024 •
edited
Loading

Thank you @martin-traverse.

  • I believe we agree on the nature/existence of the bug.
  • I believe this solution works (with some caveats).

Is there anything holding back approving this CR? Or are we to understand that this library is no longer being maintained and we should fork it? @alexander-fenster?

That said protobuf syntax="proto3" has supported the optional keyword by default since the release >3 years ago.

https://github.com/protocolbuffers/protobuf/releases/tag/v3.15.0

2021-02-05 version 3.15.0 (C++/Java/Python/PHP/Objective-C/C#/Ruby/JavaScript)

Protocol Compiler

  • Optional fields for proto3 are enabled by default, and no longer require
    the --experimental_allow_proto3_optional flag.

  • At this point, we should probably support optional by default, but help legacy users with a flag like: --legacy_ignore_proto3_optional=true.

  • In TypeScript, it doesn't look like we're doing enough to protect against non-optional fields from being omitted/set to correct types.

@martin-traverse martin-traverse force-pushed the fix/typescript_optional_support branch from 971c974 to 94556f9 Compare July 24, 2024 10:53
@martin-traverse
Copy link
Contributor Author

I have rebased this change on top of the latest master. @alexander-fenster are you ok to review / approve this change now? Is there anything else you need from me?

@martin-traverse
Copy link
Contributor Author

Hello - I am closing this PR as I have a substantially new implementation

@martin-traverse martin-traverse mentioned this pull request Jul 24, 2024
Merged
@martin-traverse
Copy link
Contributor Author

Please see #2011 for a more complete solution to this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Asondo Asondo Asondo left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@martin-traverse @mamiksik @andrea11 @zargold @Asondo