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

Some improvements to JSONSchemaType for records #1564

Merged

Conversation

erikbrinkman
Copy link
Collaborator

What issue does this pull request resolve?

fixes #1491

What changes did you make?

For JSONSchemaType: additional properties doesn't required type: "string" since it's
implied. required is no long required for objects without any required
members.

Is there anything that requires more attention while reviewing?

  1. While I think that ultimately this makes the type support better for records, It does make the type definition itself a bit messier, so I'm open to the perspective that that messiness isn't worth the improvement
  2. I chose to keep required required for partial types. I think that makes sense, but it's worth calling out.
  3. The tests are only type tests, I can add schema validation tests too if you think it's necessary

Additional properties doesn't required `type: "string"` since it's
implied. `required` is no long required for objects without any required
members.

fixes ajv-validator#1491
// are listed it only asserts that optional cannot be listed.
// "required" is not necessary if it's a non-partial type with no required keys
_partial extends true
? {required: Readonly<(keyof T)[]>}
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering - why "required" is required in partial schemas? I am probably missing something, as I write the example, it does not seem required (even when it has to be). This example type checks:

type MyUnion = {tag: "a"; a: number} | {tag: "b"; b: number}

const myUnionSchema: JSONSchemaType<MyUnion> = {
  type: "object",
  required: ["tag"],
  oneOf: [
    {
      properties: {
        tag: {const: "a"},
        a: {type: "number"},
      },
    },
    {
      properties: {
        tag: {const: "b"},
        b: {type: "number"},
      },
    },
  ],
};

but it probably it should have required: ["a"] (or "b") in subschemas...

Either way, it's unrelated issue - I am going to merge it as is - thank you very much!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it doesn't matter because because partial schemas are just that, partial:

export type PartialSchema<T> = Partial<JSONSchemaType<T, true>>

@epoberezkin epoberezkin merged commit 2edc54c into ajv-validator:master Apr 24, 2021
@erikbrinkman erikbrinkman deleted the jsonschematype-improvements branch April 24, 2021 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

JSONSchemaType support for Record
2 participants