Skip to content

feat: support minItems and maxItems #242

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

Merged
merged 13 commits into from
Jul 27, 2019
Merged

feat: support minItems and maxItems #242

merged 13 commits into from
Jul 27, 2019

Conversation

bradzacher
Copy link
Contributor

@bradzacher bradzacher commented Jul 2, 2019

NOTE - this branch was based off of #241.
Github will show a number of unrelated commits until that PR is merged.
For a nicer reviewing experience, please only review commits after 5cb467a


Fixes #218

This PR:

  • add support for generating tuple types with optional and non-optional elements if minItems / maxItems exists.
  • handles having either minItems / maxItems greater than the length of the provided items.
  • handles having maxItems less than the length of the provided items.
  • converts array types to tuple types in the presence of minItems or maxItems.

Copy link
Owner

@bcherny bcherny left a comment

Choose a reason for hiding this comment

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

This looks super good overall. A few comments. Also, what do you think about updating generated output to be more safe? As-is:

type A = [string, string?, string?]
const a: A = ['a', undefined, 'c']

Instead, we could avoid accidentally allowing undefined through:

type B = [string] | [string, string] | [string, string, string]
const b: B = ['a', undefined, 'c']

@bradzacher
Copy link
Contributor Author

I've addressed the other comments.

In regards to changing the types, I think it's a good idea.
I'll have a look at it tonight.

@bradzacher
Copy link
Contributor Author

bradzacher commented Jul 16, 2019

It makes the types verbose, but it adds a heap more type safety.
It also allows explicit assertion of the empty array state when no minItems is provided.

I'm a lot happier with the types this way!

Copy link
Owner

@bcherny bcherny left a comment

Choose a reason for hiding this comment

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

Almost there -- one last (minor) suggestion.

- fix normalizer not applying deeply
- normalize minItems
- add tests for zero min/maxItems
- fix some bugs that were surfaced from that
@bradzacher
Copy link
Contributor Author

OOOOKay. Sorry, this PR keeps growing.

So in adding in a normalizer for minItems, I noticed that the normalizer wasn't traversing correctly. I added a naive normaliser (if (!('minitems' in schema)) { schema.minItems = 0 }), and it broke most of the tests because the normalization algorithm traversed every single object key equally.

I noticed that most of the normalizers had a check for stringify(schema) === stringify(rootSchema), i.e. only apply at the root, but this means that they don't get applied correctly for anything beyond that - i.e. definitions, allOf, anyOf, oneOf, not, items, properties....

So I added a traverser which will properly traverse the schema, which allows the normalizers to correctly apply to things which are actually schemas beyond the root schema.

This surfaced a few bugs which I fixed.

Copy link
Owner

@bcherny bcherny left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks for the enhancement, and thanks for improving underlying infrastructure along the way, Brad!

@bcherny bcherny merged commit c5f4f03 into bcherny:master Jul 27, 2019
@bcherny
Copy link
Owner

bcherny commented Jul 27, 2019

Published 7.0.0.

@bradzacher bradzacher deleted the 218-minItems-maxItems branch July 27, 2019 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tuple items with minItems and additionalItems
2 participants