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: Add static-only variant to to outputTypeAnnotations option #858

Merged
merged 2 commits into from
Jul 4, 2023

Conversation

bouk
Copy link
Collaborator

@bouk bouk commented Jun 30, 2023

Hi, I want to write a generic function like such:

function Encode<D>(desc: {$type: string, encode(value: D): Writer}, value: D)

So I need to have type annotations emitted but also still have the interfaces generated without the $type field. In this PR I'm adding a const-only variant to outputTypeAnnotations that does this.

@bouk bouk changed the title Add const-only variant to to outputTypeAnnotations option feat: Add const-only variant to to outputTypeAnnotations option Jun 30, 2023
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.

Hi @bouk ! This sounds like a good idea...

Two asks:

  1. do you mind renaming this to outputTypeAnnotations=staticOnly? The "const-only" at first made me think of the TypeScript const feature, like the output would become readonly or as const or what not--but makes senses, you just want only the "static" $type field, and not one added to the actual message interface.

  2. Can you create a new integration/* directory with a parameters.txt that uses the staticOnly param, just to get some regression coverage of this feature?

Thanks!

src/main.ts Outdated
@@ -616,7 +619,9 @@ function makeDeepPartial(options: Options, longs: ReturnType<typeof makeLongUtil

// Based on the type from ts-essentials
const keys =
options.outputTypeAnnotations || options.outputTypeRegistry ? code`Exclude<keyof T, '$type'>` : code`keyof T`;
(options.outputTypeAnnotations || options.outputTypeRegistry) && options.outputTypeAnnotations !== "const-only"
Copy link
Owner

Choose a reason for hiding this comment

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

Hm, this logic is doing in enough places, can you create like a addTypeToMessages(options) helper function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@bouk
Copy link
Collaborator Author

bouk commented Jul 3, 2023

I've renamed it to static-only (to match other options), added a helper and added an integration test

@bouk bouk requested a review from stephenh July 3, 2023 15:06
@stephenh
Copy link
Owner

stephenh commented Jul 3, 2023

Cool, thanks @bouk ! Looks like there is a build failure; could you 👀 , but otherwise is great, and I'll merge once that's passing.

* Add integration test
* Add helper for adding type to message
@bouk bouk force-pushed the type-annotation-only-const branch from 7d91df6 to dc1acf2 Compare July 4, 2023 07:48
@bouk bouk changed the title feat: Add const-only variant to to outputTypeAnnotations option feat: Add static-only variant to to outputTypeAnnotations option Jul 4, 2023
@bouk
Copy link
Collaborator Author

bouk commented Jul 4, 2023

@stephenh forgot to add a parameter file!

@bouk bouk requested a review from stephenh July 4, 2023 08:20
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.

👍

@stephenh stephenh merged commit d7c4af7 into stephenh:main Jul 4, 2023
stephenh pushed a commit that referenced this pull request Jul 4, 2023
# [1.151.0](v1.150.1...v1.151.0) (2023-07-04)

### Features

* Add static-only variant to to outputTypeAnnotations option ([#858](#858)) ([d7c4af7](d7c4af7))
@stephenh
Copy link
Owner

stephenh commented Jul 4, 2023

🎉 This PR is included in version 1.151.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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants