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

feat: allow $type to be optional #1013

Merged
merged 2 commits into from
Mar 8, 2024
Merged

Conversation

lukealvoeiro
Copy link
Collaborator

@lukealvoeiro lukealvoeiro commented Mar 8, 2024

Description

Add another possible value to the outputTypeAnnotations option: optional. This makes type definitions on interfaces optional, which may be useful if you want to use the $type field for runtime type checking on responses from a server but don't want to have to set the $type on each request message you create.

Testing

Ensured this causes no regressions for existing integration tests. Also added new integration tests to prevent future regressions.

Copy link
Owner

@stephenh stephenh left a comment

Choose a reason for hiding this comment

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

This makes sense. I worry a little bit that it shows ts-proto should have a clearer distinction of "read types" vs. "creation types".

I.e. my assertion/understanding would be that:

  • Anyone reading / accepting a message type would generally prefer $type to not be optional, and always be required, b/c it simplifies their code, without having to check "if $type set/not set".

  • Anyone creating a message type would probably prefer to leave $type unset, and like a SomeMessage.create({ ... }) fill in the $typefor them, similar to how create can set field defaults.

In theory, ts-proto does have "read types" (just the interface) and "write types" (the Partials that are accepted by SomeMessage.create, but I think in practice the allure / primary feature of ts-proto is precisely that message types are "just interfaces" (not the usual classes that protobuf frameworks usually use), and the "just interfaces" approach is just kinda fundamentally at odds with the "read type vs. write type" nuance.

So, dunno, just some "fwiw" background on why I'd preferred to keep $type required so far, but it's a really simple change, and so whatever you find most ergonomic for your codebase sgtm!

Thanks!

(I'll approve & merge this PR as a formality, but if you're interested feel free to start approving/self-merging your own PRs (once they're passing tests / you feel comfortable with the changes / etc); it'd be great to have another maintainer helping out with the project!)

@stephenh stephenh merged commit f285557 into main Mar 8, 2024
6 checks passed
@stephenh stephenh deleted the lalvoeiro/allow-optional-$type branch March 8, 2024 15:13
stephenh pushed a commit that referenced this pull request Mar 8, 2024
# [1.168.0](v1.167.9...v1.168.0) (2024-03-08)

### Features

* allow `$type` to be optional ([#1013](#1013)) ([f285557](f285557))
@stephenh
Copy link
Owner

stephenh commented Mar 8, 2024

🎉 This PR is included in version 1.168.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

lukealvoeiro added a commit that referenced this pull request Mar 12, 2024
* main:
  chore(release): 1.168.0 [skip ci]
  feat: allow `$type` to be optional (#1013)
  chore(release): 1.167.9 [skip ci]
  fix: typescript errors for struct with optional=all (#1008)
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.

None yet

2 participants