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

Write null instead of undefined #869

Closed
tomasweigenast opened this issue Jul 8, 2023 · 3 comments · Fixed by #1017 · May be fixed by Graysonbarton/synthetics-sdk-nodejs#2 or VaniHaripriya/data-science-pipelines-tekton#1
Closed
Labels
enhancement New feature or request help wanted Extra attention is needed released

Comments

@tomasweigenast
Copy link

A good option to implement can be the possibility to compare with null instead undefined. This is useful when dealing with Firestore, which skips undefined properties and can generate issues when sorting.

@0xRapid
Copy link

0xRapid commented Aug 13, 2023

I was just about to open an issue and I saw this!

This is a must-have in my opinion. In my company we all work with null types since undefined can thrown in some cases, Working with null is more convenient to know if it was the developer's intention or unknown.

From my searching, there have been multiple issues about it, what do you say @stephenh :p

@stephenh
Copy link
Owner

Is the idea that if Author is a message, and Author.address is an unset child message, that console.log(author.address) would print null?

Fwiw normally in non-protobuf/JSON APIs, I like to use child: null to be "unset the child" and child: undefined (or just omitted) to mean "leave child alone", so I get the intuition for "null more intentionally means unset/empty".

That said, protobuf doesn't have a way to differentiate these two on the wire, so that null-is-unset / undefined-is-omit convention is (unfortunately imo) not applicable to protobuf APIs.

Granted, Firestore and other "null is just our preferred convention" is fair, but personally not something I'll work on.

If you'd like to submit a PR, and the PR is not huge, I'd consider accepting it as yet-another-flag...that said, I'm worried such a fundamental change is going to be a lot of complications to the codebase, but who knows, maybe not.

@stephenh stephenh added enhancement New feature or request help wanted Extra attention is needed labels Aug 22, 2023
stephenh pushed a commit that referenced this issue Mar 30, 2024
Changes:
- with this flag, undefined types will be replaced with null.
- fields with optional label in proto files, will implicitly accept
undefined too.

this feature is needed when we wanna have better type alignment with
ORMs ( drizzle, typeorm, prisma ) or other services such as Firestore,
since they mostly ignore `undefined` in their types.

Note:

@stephenh as you wanted I made simple, small changes, this works well
for my `nestjs` project, I'm not very familiar with other
implementations and frameworks, please check it out and let me know what
you think. if any changes are required please let me know, I will work
them out.

thanks for this amazing library 🙏 

closes #869
stephenh pushed a commit that referenced this issue Mar 30, 2024
# [1.171.0](v1.170.0...v1.171.0) (2024-03-30)

### Features

* added useNullAsOptional option ([#1017](#1017)) ([573f63e](573f63e)), closes [#869](#869)
@stephenh
Copy link
Owner

🎉 This issue has been resolved in version 1.171.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed released
Projects
None yet
3 participants