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

Treat null as undefined when generating protobuf messages #266

Open
markh123 opened this issue Apr 7, 2022 · 2 comments
Open

Treat null as undefined when generating protobuf messages #266

markh123 opened this issue Apr 7, 2022 · 2 comments
Labels
enhancement New feature or request

Comments

@markh123
Copy link
Contributor

markh123 commented Apr 7, 2022

Can we treat null the same as undefined when creating protobuf message objects? Something like the following diff:

--- a/packages/runtime/src/reflection-merge-partial.ts
+++ b/packages/runtime/src/reflection-merge-partial.ts
@@ -29,20 +29,20 @@ export function reflectionMergePartial<T extends object>(info: MessageInfo, targ

         if (field.oneof) {
             const group = input[field.oneof] as UnknownOneofGroup | undefined; // this is the oneof`s group in the source
-            if (group === undefined) { // the user is free to omit
+            if (group == undefined) { // the user is free to omit
                 continue; // we skip this field, and all other members too
             }
             fieldValue = group[name]; // our value comes from the the oneof group of the source
             output = (target as UnknownMessage)[field.oneof] as UnknownOneofGroup; // and our output is the oneof group of the target
             output.oneofKind = group.oneofKind; // always update discriminator
-            if (fieldValue === undefined) {
+            if (fieldValue == undefined) {
                 delete output[name]; // remove any existing value
                 continue; // skip further work on field
             }
         } else {
             fieldValue = input[name]; // we are using the source directly
             output = target as UnknownMessage; // we want our field value to go directly into the target
-            if (fieldValue === undefined) {
+            if (fieldValue == undefined) {
                 continue; // skip further work on field, existing value is used as is
             }
         }

I know that based on the typescript types defined for protobuf objects we should not be passing null as a value for protobuf fields. However, I am in the process of migrating over to use protobuf-ts and this scenario (values being set to null) does seem to crop up occasionally. For example, when reading data from a database it is possible that many values are null and I am currently needing to convert them to undefined before I can translate the objects into a protobuf response.

@timostamm timostamm added the enhancement New feature or request label Apr 12, 2022
@timostamm
Copy link
Owner

Treating null the same as undefined as suggested seems like a good idea to me. It will be a type error because PartialMessage is typed for undefined, but it will make creating a new message more robust.

@gkolotelo
Copy link

gkolotelo commented Apr 24, 2022

I think this change would be beneficial too. I'm using the Sequelize ORM and it returns null on unset DB associations. It'd make it much easier to just pass the object to the create function and let protobuf-ts handle creating the respective messages when the associations exist and skipping the fields when they are null or undefined.

In my case I ended up modifying the generated created methods, by replacing the generated if (value !== undefined) with if (value != undefined). In this way I can just run a batch "fix" over the protoc generated codebase with the following command:

find ${FRONTEND_OUT_DIR} -name "*.ts" | xargs sed -i 's/if (value !== undefined)/if (value === null) return undefined; if (value != undefined)/g'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants