Feature: All Types as Readonly

[craigmiller160]

Related To: #214

Rationale

Regardless of the OpenAPI specification for the types, when writing code with any types there are advantages to enforcing readonly on them. This is one piece of immutable programming design, where variables should never change after their initial declaration/assignment. Such an approach offers a variety of benefits in terms of readability and maintainability, and is the foundation of Functional Programming design patterns.

Adding the option to make all generated types readonly allows for users of this library who prefer to program with immutable design patterns to more easily integrate the output into their codebases.

Alternatives

It has been said that the OpenAPI schema definition could either be a) made readonly at the source, or b) parsed prior to passing into this library to make everything readonly. While this is certainly possible, there are disadvantages to this approach.

First, many OpenAPI schemas consumed by frontend applications are either not directly controlled by the frontend developer or are auto-generated by tooling in which this kind of fine-grained modification is not straightforward. While having the source OpenAPI schema be as accurate as possible is always an ideal solution to all openapi-type-generation efforts, ensuring that the schema enforces readonly across the board is not a solution that can be relied upon in all cases.

As for processing the schema prior to submitting it to this library, that is absolutely a solution that could be reliable in all cases. However, it puts more work on the developer consuming this library to take those preparations. In general, it would be highly beneficial if this library could automatically perform that operation without any pre-processing of the schema.

Implementation Overview

1. Readonly behavior means that all properties of an object are readonly, and all arrays are ReadonlyArrays.
2. A new option, allReadonly, has been added to this tool. When unused, it defaults to false. When set to true, all Zod schemas and generated TypeScript types will enforce readonly behavior across the board.
3. The option is available programmatically, or as a new CLI argument, --all-readonly.
4. Various tests have been added to the codebase to validate the new behavior. All existing tests, that don't use this argument, also pass, proving that existing behavior is not affected.

Implementation Quirk - Readonly Arrays

This library contains the ability to generate TypeScript types as well as Zod schemas. When generating the TypeScript types, the Tanu library is used. I am new to Tanu, so if there is a better way to implement this that resolves the following quirk please recommend it. I was unable to find an alternative myself.

The Tanu library does not have the equivalent of t.readonlyarray(). As a result, to make an array readonly, I had to write t.readonly(t.array()). This has two impacts on the output.

The first is that the type produced is Readonly<Array<T>>, instead of the more traditional ReadonlyArray<T>. While the latter is a bit cleaner IMO, the two types are identical and inter-changeable in all practical ways.

The second is that, if an array is a property in an object, it will be double-readonly. All objects wrapped are generated as Readonly<{}>, rather than assigning the readonly modifier to each property. However, this approach to making an array readonly will add the readonly modifier to the relevant property if it is a member of an object. To further clarify, this is an example of the output:

export type TransactionsPageResponse = Readonly<{
	readonly transactions: Readonly<Array<TransactionResponse>>;
	pageNumber: number;
	totalItems: number;
}>;

Yet again, from a practical perspective this does not have any impact on the functionality of the type. It is simply a weird quirk of how the Tanu library generates the type outputs.

Conclusion

I hope that this PR meets the standards of this project, and can be speedily adopted into what is already an excellent tool for the TypeScript community.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
openapi-zod-client	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 2, 2023 6:37pm

[astahmer]

LGTM

just a comment on this part:

The Tanu library does not have the equivalent of t.readonlyarray(). As a result, to make an array readonly, I had to write t.readonly(t.array()). This has two impacts on the output.

if that output doesn't satisfy you, you can very well make your own const readonlyArray => (t: string) => ... that outputs exactly what you prefer

[craigmiller160]

Thank you so much for your feedback. Two items:

1. Can you provide more details on how to compose that custom readonlyArray function with the Tanu library? Or provide a link to better documentation for how to work with it? If it is straightforward to adjust this behavior, I would happily do so.

2. Other than the above question related to readonly arrays, do you have any opposition towards merging this PR? Otherwise, once the above discussion is complete, will the change be merged and released?

[astahmer]

oh, I meant that you can do so without using tanu. like the example fn I wrote above

and yes, I'm fine with merging as it is, given this doesn't break/impact current behaviour/users and is "just" an addition, I don't see a reason not to have it if it can help some people

thanks for your work on this !

[astahmer]

I do have 1 tiny request before merging
can you move the readonly tests AFTER the current tests ?
https://github.com/astahmer/openapi-zod-client/pull/221/files#diff-757bee93163a4b15e04c6dc901d34861037402bb390ef1e4f5ec828eb17371b6R13
https://github.com/astahmer/openapi-zod-client/pull/221/files#diff-2ed0ebee5b7cbec7f336b958ae8ef2df6dd6ce2495c1938baa38118c48ef68a2R328

so that other contributors can see the default behaviour first

[craigmiller160]

Of course. I have made the requested change to the order of the tests. Feel free to merge this at your convenience.

Once this is merged, what is your release schedule? Or do you perform fully automated CI/CD releases? I only ask because obviously I'm eager to use this in my own personal work. :)

Thank you again for your support of this addition to the project.

[astahmer]

Once this is merged, what is your release schedule? Or do you perform fully automated CI/CD releases? I only ask because obviously I'm eager to use this in my own personal work. :)

ah, good catch. we use changesets so you can add one with patch (since it doesn't change current behaviour) and once this PR is merged, the changeset github action workflow will automatically create a "Version packages" PR, that once merged will release it on npm

[craigmiller160]

Ok. Apologies for recapping, I haven't used this changeset tool before to drive a release process.

1. I would be the one in this case to initiate the changeset "patch", ie bumping the version by x.x.1.
2. Would I do this in the current PR, or in a new PR?
3. Would I do this using the changeset cli too, or would simply changing the version in the package.json manually be enough?
4. Anything else I should know?

Thank you again.

[astahmer]

1 - yes
2 - in the current PR
3 - using the changeset CLI, this will generate a special changeset file in which you can insert a little bit of a recap/docs on what you're adding, the impacted monorepo packages (only the lib here) and the kind of version bump it should be for each of them (patch here)
4 -no I think that's it !

[craigmiller160]

Excellent. One final question: shouldn't it be a minor version bump (x.1.x) and not a patch version bump (x.x.1). My understanding of semantic versioning is that new functionality that is 100% backwards compatible would mean a minor version bump, whereas patch version bumps are just bug fixes to existing functionality.

Its your project, I'll version it however you see fit. Just my two cents.

[astahmer]

both are fine with me tbh, I don't have a strong opinion here
I personally don't 100% follow the semantic versioning thing otherwise we'd be at version 10+ 😅

if you feel like a minor is more appropriate, that's fine with me ! after all, that's one of the goals of the changesets, leaving that decision to the contributors

[craigmiller160]

Ok, I believe I have made the correct changeset update. Can you confirm, and if so merge? If I made a mistake with the changeset, please inform me and I will correct it. Thank you again.