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: option to declare schema as const #1096

Merged
merged 4 commits into from
Sep 4, 2024

Conversation

cmd-johnson
Copy link
Contributor

This PR adds a new option that allows us to get additional type information for the protoMetadata export when using outputSchema=true.

Background

Using the outputSchema=true option, ts-proto outputs a protoMetadata export for all proto files.

This metadata object can be useful for rudimentary reflection and required if we want to access value options.
Unfortunately there is no compile time type safety for these values, because the export is declared as ProtoMetadata, losing a lot of useful type information.

Proposed Solution

When setting the option outputSchemaAsConst=true, do the following:

Instead of doing export const protoMetadata: ProtoMetadata = { ... }, export it as export const protoMetadata = { ... } as const satisfies ProtoMetadata.

This retains the full value of protoMetadata on the type level.
This additional type information can then be used to write type safe reflection methods, like for accessing the values of value options declared in proto files (see the included tests for some basic examples).

In order to provide type safety for the fileDescriptor field, I had to remove the call to FileDescriptorProto.fromPartial(...). As far as I can tell this call doesn't do much in practice, because the data passed to it at code generation time was already passed through FileDescriptorProto.fromPartial(fileDesc) and I think this results in the exact same value, making the fromPartial call in the generated code basically a no-op. You might want to double-check this before merging because this change isn't hidden behind the outputSchemaAsConst option.

Considerations

satisfies operator

The satisfies ProtoMetadata part of the declaration isn't strictly necessary as the exported value should match the ProtoMetadata interface anyway.
However, it can act as a hint to developers that the export conforms to the ProtoMetadata type instead of being completely independent.
It also acts as a kind of sanity check when type checking, making sure the generated type matches our expectations.

The satisfies operator is only supported since TypeScript 4.9, so code generated using this option will not work on older TS versions.

type complexity

When declaring protoMetadata as const, the type definition blows up a lot, because the whole value is now also encoded on the type level.
This allows for advanced use cases (like typesafe reflecton), but could potentially slow down the compiler when dealing with large schemas.
It also increases type declaration file sizes when the typescript files when the resulting .ts files are compiled to .js and .d.ts files.

@cmd-johnson cmd-johnson force-pushed the feature/meta-typings-as-const branch from aedb4e5 to 77b580d Compare August 20, 2024 15:45
@cmd-johnson cmd-johnson force-pushed the feature/meta-typings-as-const branch from 77b580d to ba318c7 Compare September 2, 2024 12:35
@cmd-johnson
Copy link
Contributor Author

Looks like the previous CI errors were because the tsconfig.proto.json used for validating the integration test types extends @tsconfig/strictest/tsconfig.json. This enables the exactOptionalPropertyTypes option, which in turn results in a few instances of generated code failing type checking because they assigned undefined in places were an object property was declared as propName?: T (namely in service and enum options).

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.

Just one musing question about the config flag, but otherwise looks great; thanks @cmd-johnson !

README.markdown Outdated
@@ -456,6 +456,8 @@ Generated code will be placed in the Gradle build directory.

- With `--ts_proto_opt=outputSchema=true`, meta typings will be generated that can later be used in other code generators. If outputSchema is instead specified to be `no-file-descriptor` then we do not include the file descriptor in the generated schema. This is useful if you are trying to minimize the size of the generated schema.

- With `--ts_proto_opt=outputSchemaAsConst=true`, the `protoMetadata` export of `outputSchema=true` is declared using `as const` and `satisfies` (supported by TypeScript since [4.9](https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-9.html#the-satisfies-operator)), providing more detailed type information.
Copy link
Owner

Choose a reason for hiding this comment

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

Hi @cmd-johnson ! Wdyt of making outputSchema a tristate, like outputSchema=const would enable this behavior, vs. a net-new flag? We've done that with a few other flags and it's seemed nice so far.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I wanted to do at first, but outputSchema already can be true or no-file-descriptor, which would mean that adding an as const option would also require adding a no-file-descriptor-as-const option or make the option's type an array of a new OutputSchema enum (similar to how outputServices works) and I didn't want to update all usages of no-file-descriptor 😅.

If you'd rather go that route than adding another parameter, I can make those changes. I'd prefer going the enum array route in that case, because it scales better if more options get added to the outputSchema flag at some point in the future.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah yep! I was wondering if we'd run into that.

The enum array of outputSchema=no-file-descriptor,const,etc approach would be great, since you don't mind; thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!
I've made it so you can combine the flags by specifying it multiple times, which is the way the outputServices flag works:

--ts_proto_opt=outputSchema=const,outputSchema=no-file-descriptor

// chaining if we didn't use `as const` to declare the `protoMetadata`
// export.

expect(protoMetadata.options.enums.TestEnum.values.VALUE_A.string_value).toBe("A");
Copy link
Owner

Choose a reason for hiding this comment

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

Ah nice! Great test, and I see what you're wanting to achieve

@stephenh
Copy link
Owner

stephenh commented Sep 4, 2024

Looks great, thanks @jonaskello !

@stephenh stephenh merged commit 4cc1a1e into stephenh:main Sep 4, 2024
6 checks passed
stephenh pushed a commit that referenced this pull request Sep 4, 2024
# [2.1.0](v2.0.4...v2.1.0) (2024-09-04)

### Features

* option to declare schema as const ([#1096](#1096)) ([4cc1a1e](4cc1a1e))
@stephenh
Copy link
Owner

stephenh commented Sep 4, 2024

🎉 This issue has been resolved in version 2.1.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