-
Notifications
You must be signed in to change notification settings - Fork 396
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
Add support for readonly types #412
Conversation
…Keyword option. Updated README.
…ypes, readonly properties, and the default readonly property state.
…same property schema.
…ty weren't allowed to be readonly.
What would be the downside of using JSON schema's interface Foo {
readonly arr: readonly string[]
} I agree that readonly support would be great. However, our team generates interfaces/classes in multiple languages from the same OpenAPI spec. We'd like to avoid having a bunch of language specific, non-official-OpenAPI attributes in our specs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thanks for putting up this PR and sorry for taking so long to give feedback on it. I appreciate the work and thoroughness you've put into it!
In general, I'm supportive of supporting readOnly
. However, I'd suggest taking a slightly different approach:
- We should support the standard JSON-Schema
readOnly
keyword. We should not introduce our own JSTT-specific keywords, or add new options to JSTT. To address the questions in your summary: a breaking change + major version is the right approach here; the$ref
behavior in this PR is correct; and the output I'd expect for your cases is:
// Input (1)
{
"type": "object",
"additionalProperties": {
"type": "array",
"items": {
"type": "string"
},
"readOnly": true
}
}
// Output (1)
interface Test {
readonly [k: string]: string[];
}
// Input (2)
{
"type": "object",
"additionalProperties": {
"readOnly": true,
"type": "array",
"items": {
"type": "string"
}
}
}
// Output (2)
interface Test {
[k: string]: readonly string[];
}
- For your tests, you did add a lot and once again I appreciate the attention to detail you put into it. However, I'd suggest combining tests a bit more to avoid testing unnecessary cases. Off the top of my head, the key cases you want to test are (am I missing anything?):
readOnly
properties: objects, arrays, tuples, primitives, etc.readOnly
definitionsreadOnly
xallOf
readOnly
xallOf
, where each branch of theallOf
has the same property, but just one of the branches is markedreadOnly
(correct behavior per the spec: the result should bereadOnly
)readOnly
xanyOf
See https://github.com/bcherny/json-schema-to-typescript/pull/353/files, if you'd like to combine its approach with the one in this PR.
/** | ||
* Makes Windows paths look like POSIX paths, ignoring drive letter prefixes (if present). | ||
*/ | ||
export function forcePosixLikePath(path: string): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull this into a separate PR, please.
Closing out stale PRs. Feel free to re-open if you'd like to revisit this. |
This PR adds support for
readonly
types in the TypeScript output.Details
Adds 3 new custom schema properties:
"tsReadonly"
, controlling whether an array or tuple type is readonly."tsReadonlyProperty"
, controlling whether a property in an object schema is readonly."tsReadonlyPropertyDefaultValue"
, which (when used on an object schema) sets the default value of"tsReadonlyProperty"
properties on this object's properties (yes this sounds confusing, more on this later).(I talk about why I'm not using the JSON Schema spec's
"readOnly"
property later).Adds 2 new options:
readonlyByDefault
, controlling the default values of"tsReadonly"
and"tsReadonlyProperty"
. Defaults tofalse
.readonlyKeyword
, controlling whether to write readonly array types asreadonly X[]
orReadonlyArray<X>
. Defaults totrue
. If set tofalse
tuple types will always be mutable, regardless of other factors (more details later).Property
"tsReadonly"
boolean
This property, when used on an array schema, controls whether said array schema should compile to a readonly TypeScript array/tuple type.
If unspecified, inherits its value from the
readonlyByDefault
option.Examples
Property
"tsReadonlyProperty"
boolean
This property, when used on a property of an object schema, controls whether or not said property should be readonly in Typescript. Supports properties in
"properties"
and"patternProperties"
, as well as"additionalProperties"
.If unspecified, inherits its value from:
"tsReadonlyPropertyDefaultValue"
property, if specified.readonlyByDefault
option.Examples
Why a Different Property?
We need different properties to talk about readonly array schemas vs. readonly properties because if we didn't the following case would be ambiguous:
Was the schema author's intent to mark the array schema as readonly, outputting
{[k: string]: readonly string[]}
, or was it to mark the property as readonly, outputting{readonly [k: string]: string[]}
? Or maybe it was both? We can't tell with just one property, hence the need for two.Property
"tsReadonlyPropertyDefaultValue"
boolean
This property, when used on an object schema, controls the default value for the
"tsReadonlyProperty"
properties on the object schema's properties. Essentially it's like applyingReadonly
to the entire object type, with the difference that properties can explicitly be marked as mutable.Like
Readonly
, the value of this properly only applies to the immediate object schema. Sub-schemas, such as those defining the types of certain properties, are not affected by this property (unless said sub-schema is itself an object schema which specifies this property).This property applies to immediate sub-schemas in
"properties"
and"patternProperties"
, as well as"additionalProperties"
.This property technically has no default value, but based on code output it can be said to have an imaginary default value matching the value of the
readonlyByDefault
option (see the"tsReadonlyProperty"
value inheritance chain).Examples
Option
readonlyByDefault
boolean
false
This option sets the default value for
"tsReadonly"
properties, and for"tsReadonlyProperty"
properties that are not otherwise affected by"tsReadonlyPropertyDefaultValue"
. This can be thought of as the value of last resort for the first two properties.This option defaults to
false
to preserve backwards compatibility.Option
readonlyKeyword
boolean
true
This option controls whether array schemas compile to
readonly X[]
orReadonlyArray<X>
. Iftrue
it compiles using thereadonly
keyword.This option defaults to
true
because the TypeScript version that thereadonly
keyword was introduced in (3.4) is reasonably old, and setting it tofalse
leaves us with no way to write a readonly tuple type (more on that in the next subsection).The Tuple Type Problem
readonly
as a type keyword was first introduced in TypeScript 3.4.References:
The problem with this is that people with codebases stuck in earlier versions of TypeScript (for whatever arcane legacy code reason) don't have a way to represent readonly tuple types. As such, when
readonlyKeyword
isfalse
, we have no choice but to fall back to outputting a mutable tuple type;readonly [string, ...string[]]
becomes[string, ...string[]]
. We could possibly throw an error instead, but I felt that this way was safer so that people who setreadonlyByDefault
totrue
at least get something as-is, instead of having to go through and explicitly specify"tsReadonly": false
for every tuple type (or turn off tuple types altogether).Notes on
"readOnly"
The JSON Schema specification defines its own property -
"readOnly"
- to deal with at least some of what the custom properties in this PR do. Why not use it? Well..."tsReadonlyProperty"
)."readOnly"
is a standard JSON Schema property, using it to extract information about TypeScript readonly-ness is a breaking change and would require a major version increment.I'm completely open to discussion regarding making use of
"readOnly"
in some way for TypeScript readonly detection, but I felt it safer to implement it like this for now.Notes on Test Cases
I have written end-to-end tests to cover (almost all) combinations of property values, options, and inheritance chains.
That's a lot of test files. Like, 122.
If you feel that some of these tests are unnecessary or overkill feel absolutely free to delete them, or merge them all into one mega test file if that's something that's supported (I couldn't figure out how to do it). Or tell me to do it, I'm also fine with that.
Also feel free to rename them; the names are also stupidly long.
A Question Regarding
"$ref"
"$ref"
behaves a little bit strange because of the dereferencing step.The following schema:
Compiles to:
I could fix this by
delete
ing"tsReadonlyProperty"
off of dereferenced refs, but I'm unsure if the current behaviour is actually desirable - it certainly would save some characters, and overriding it in the schema containing the"$ref"
property does work. I'm leaving this as-is pending feedback, and as such there is currently no test case for this behaviour.An Additional (Properties) Bugfix
As a side-effect of the main work done on this PR a bug with "additionalProperties" and custom properties was fixed.
Before
After
This fix requires active maintenance if additional custom properties are defined - said custom property must be added as an exception in the
nonCustomKeys
function insrc/typesOfSchema.ts
.A test case was added to enforce this bugfix.