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: toJSON Function with removeEnumPrefix=true and unrecognizedEnumValue=0 Options #1089

Conversation

kanziw
Copy link
Contributor

@kanziw kanziw commented Aug 15, 2024

close: #1086

This PR addresses the issue described in #1086, where the toJSON function generated by ts-proto incorrectly serializes enum values when both the removeEnumPrefix=true and unrecognizedEnumValue=0 options are used.

The expected behavior is that, even with removeEnumPrefix=true, the toJSON function should encode the enum values using their original string values, including the prefix. However, the bug causes the function to encode the enum values without the prefix, leading to incorrect JSON serialization.

To fix this, the PR updates the toJSON function to correctly use the original enum name, including the prefix, when encoding. This is achieved by storing the originalName alongside the UnrecognizedEnum value and using it during serialization.

This change ensures that the enum values are correctly serialized with their full names, aligning the behavior with the expected output when these options are enabled.

If this code format doesn't align with the maintainer's preferences, please feel free to modify it as needed.

@kanziw kanziw changed the title Fix toJSON Function Bug with removeEnumPrefix=true and unrecognizedEnumValue=0 Options (#1086) Fix toJSON Function Bug with removeEnumPrefix=true and unrecognizedEnumValue=0 Options Aug 15, 2024
@kanziw
Copy link
Contributor Author

kanziw commented Aug 15, 2024

When I first tried to contribute to this project, it took me quite some time to figure out how to write and run test cases. I would like to suggest creating a CONTRIBUTING document to share with the community, so that others can more easily contribute to the project.

Based on my experience, the document could include the following steps:

  1. Create a directory under integration with an appropriate TEST_NAME.
  2. Write the proto file for your test in $TEST_NAME.proto.
  3. Specify the necessary ts-proto options in parameters.txt.
  4. Run yarn proto2ts $TEST_NAME to generate the stubs.
  5. Using the generated stubs, write your test cases in $TEST_NAME-test.ts.
  6. Continue writing your code until the test passes by running yarn test $TEST_NAME-test.ts.

@kanziw kanziw changed the title Fix toJSON Function Bug with removeEnumPrefix=true and unrecognizedEnumValue=0 Options fix: toJSON Function with removeEnumPrefix=true and unrecognizedEnumValue=0 Options Aug 15, 2024
@stephenh
Copy link
Owner

This is a great fix @kanziw , thank you!

Definitely hear you on the CONTRIBUTING suggestion; this Development section of the readme is supposed to help with that:

https://github.com/stephenh/ts-proto?tab=readme-ov-file#development

But the readme is quite long.

If you'd like to pull that #development section out into a CONTRIBUTING.md, and clean up it based on your fresh eyes, that would be an amazing PR as well! Thank you!

@stephenh stephenh merged commit 2401490 into stephenh:main Aug 15, 2024
6 checks passed
stephenh pushed a commit that referenced this pull request Aug 15, 2024
## [1.181.2](v1.181.1...v1.181.2) (2024-08-15)

### Bug Fixes

* toJSON Function with `removeEnumPrefix=true` and `unrecognizedEnumValue=0` Options ([#1089](#1089)) ([2401490](2401490)), closes [#1086](#1086) [#1086](#1086)
@stephenh
Copy link
Owner

🎉 This PR is included in version 1.181.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@kanziw kanziw deleted the fix-remove-enum-prefix-unrecognized-enum-value-combination-bug branch August 16, 2024 02:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug in toJSON Function with removeEnumPrefix=true and unrecognizedEnumValue=0 Option Combination
2 participants