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

Add switch to disable the minItems/maxItems expanding #250

Closed
YuJianrong opened this issue Aug 5, 2019 · 10 comments · Fixed by #274
Closed

Add switch to disable the minItems/maxItems expanding #250

YuJianrong opened this issue Aug 5, 2019 · 10 comments · Fixed by #274

Comments

@YuJianrong
Copy link

In 0.7.0 a new PR #242 is merged to support minItems/maxItems for the array, this is great!

Until something like

        "processes": {
          "type": "array",
          "items": {
            "$ref": "#/definitions/Process"
          },
          "minItems": 1,
          "maxItems": 512,

In my JSON Schema...

This kind of schema generate a HUGE interface file which is not usable. May I ask can you add a flag to disable this feature or do some limitation like convert only for the case less than 5 items? Thanks.

@bcherny
Copy link
Owner

bcherny commented Aug 5, 2019

Hey there! What issue are you running into with those big generated types — what’s “unusable” about them?

@YuJianrong
Copy link
Author

@bcherny Because the TS interfaces we generated are used for human reading too.
30k lines of interface may still friendly to the compiler, but not friendly for human. And we will commit the interface file to git, it seems it's not a good idea to commit this kind of HUGE file into git repo.

@bcherny
Copy link
Owner

bcherny commented Aug 6, 2019

Thanks for the context @YuJianrong, that does seem fair to me.

@bradzacher What do you think of bailing out at some sensible number of maxItems (say, 5, 10, or 20) and falling back to a less safe array type, since big tuple types aren’t buying us much safety anyway (since you’re not going to manually construct 500-member tuple types, and are going to have to lug around the generated tuple type everywhere you use it for little benefit)?

@bradzacher
Copy link
Contributor

probably a good idea, yeah.
I didn't think about the fact that people would supply such large numbers for maxItems.

It's probably pretty much impossible to provide values to those array types when they're that large as well.

maybe like 10 is a good round number? feels like you wouldn't really be providing more tuple elements than that by hand.

@bcherny
Copy link
Owner

bcherny commented Aug 6, 2019

That sounds good to me. :)

@YuJianrong want to contribute a fix for this? I would:

  1. Add a normalizer step to delete the minItems and maxItems properties when (maxItems - minItems) is >10.
  2. Log out a warning when you do that.
  3. Add a unit test for it.

@aslupik
Copy link

aslupik commented Oct 1, 2019

A switch to disable it completely would be very appreciated. We have a bunch of code written that expects T[] and it's a painful breaking change to adapt all of that to deal with [T, ...T[]] instead. This basically means we're not upgrading to version 7.

@lukeggchapman
Copy link
Contributor

lukeggchapman commented Jan 13, 2020

We have a problem where the union tuple types it generates are hindering our ability to manipulate the array using map and reduce.

type Test =
  | [{ test1: boolean; test2: string }]
  | [{ test1: boolean; test2: string }, { test1: boolean; test2: string }];

const arr = [
  { test1: true, test2: 'test2' },
  { test1: true, test2: 'test2' },
] as Test;

const test2 = arr.reduce(
  (previousValue, currentValue) => previousValue && currentValue.test1, // error 1
  true
);

const test3 = arr.map(val => val.test1); // error 2

Error 1:
image

Error 2:
image

I would suggest if typescript doesn't have proper support for min and max on arrays, then we shouldn't attempt to work around the issue with tuples.

@bradzacher
Copy link
Contributor

I would suggest if typescript doesn't have proper support for min and max for array then we shouldn't attempt to work around the issue with tuples.

An option would be better. I think there are valid uses for using tuples, like validating the structure of configs.

@bcherny
Copy link
Owner

bcherny commented Jan 13, 2020

Thanks for the feedback everyone! Two issues here:

  1. We should better handle really big maxItems
  2. We should add a flag to generate arrays instead of tuples when minItems/maxItems is set, at least until/if TS issues that make calling generic functions on unions painful are fixed (I think there are a few issues in TSC being tracked for this, eg. Call signatures of union types microsoft/TypeScript#7294).

For (2), if someone wants to contribute an option ignoreMinAndMaxItems that removes minItems and maxItems in a normalizer step, I'll be happy to review.

@lukeggchapman
Copy link
Contributor

@bradzacher Sure, I'm just being biased towards my use case.

Thanks @bcherny, we'll try and find some bandwidth to implement your suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants