Skip to content

fix: Backport consistency and correctness fixes (7.x)#2294

Merged
dcodeIO merged 1 commit into
protobufjs-v7.xfrom
patch/consistency-7.x
May 30, 2026
Merged

fix: Backport consistency and correctness fixes (7.x)#2294
dcodeIO merged 1 commit into
protobufjs-v7.xfrom
patch/consistency-7.x

Conversation

@dcodeIO

@dcodeIO dcodeIO commented May 30, 2026

Copy link
Copy Markdown
Member

Backports #2287, #2293 and #2275 to 7.x.

@dcodeIO dcodeIO requested a review from alexander-fenster May 30, 2026 19:59
@dcodeIO dcodeIO merged commit a92f72e into protobufjs-v7.x May 30, 2026
6 checks passed
@dcodeIO dcodeIO deleted the patch/consistency-7.x branch May 30, 2026 22:28
@UnderKoen

UnderKoen commented Jun 9, 2026

Copy link
Copy Markdown

Hi @dcodeIO , I think backporting #2287 is incorrect as failing for Message.fromObject(null) is a change in contract and should not happen in a minor version change.

This causes errors for us in nestjs where the null value cannot be converted into google.protobuf.Empty.

I feel like returning null or {} is approperiate especially for google.protobuf.Empty

@dcodeIO

dcodeIO commented Jun 9, 2026

Copy link
Copy Markdown
Member Author

Did a quick check, and you are right that this used to work for fieldless messages. That behavior was accidental, though: null is not an empty message; {} is.

If needed, a narrow compatibility fix here could be to allow null precisely for fieldless messages, but I am not yet convinced that preserving this accidental behavior of accepting an invalid value is the right tradeoff? Turns out this is a relatively simple fix.

@UnderKoen

Copy link
Copy Markdown

For my use case it doesn't really matter what is return, just as long as it does not throw an error.

It may be that @grpc/proto-loader should handle the null case better.

We ran into this problem because of our nestjs implementation where we implement function which return google.protobuf.Empty as void an there for it returns undefined into the flow and throws :/

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants