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

Implement IsArray decorator recipe #17

Merged
merged 1 commit into from
Jan 21, 2021
Merged

Implement IsArray decorator recipe #17

merged 1 commit into from
Jan 21, 2021

Conversation

jessemyers
Copy link
Contributor

  • Adds IsArray decorator recipe
  • Incorporates recipe into existing flavors
  • Normalizes some options parameters (breaking change)

 -  Adds `IsArray` decorator recipe
 -  Incorporates recipe into existing flavors
 -  Normalizes some options parameters (breaking change)
@@ -134,6 +154,20 @@ Object {
"nullable": true,
"type": "number",
},
"nullableObjectArrayValue": Object {
"items": Object {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notice that we are correctly producing JSON Schema types with items, both with a $ref (here) and a primitive (next).

@@ -51,6 +52,16 @@ export function createFixtures(
class Example {
/* Create one property of each type that is required. */

@IsArray({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's important to have examples using primitive and object types because these are handled differently.

requiredObjectArrayValue!: NestedRequiredExample[];

@IsArray({
type: Number,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no nice way to skip the type argument here and trying to compose @IsArray with IsNumber is an engineering challenge, especially since I didn't plan for it.

@@ -69,7 +80,7 @@ export function createFixtures(
requiredIntegerValue!: number;

@IsNested({
nested: NestedRequiredExample,
type: NestedRequiredExample,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For consistency, I'm now using type instead of nested. This is a break change, but since there's very little usage so far, I think it's ok.


export interface ArrayOptions {
type: Constructor,
maxItems?: number,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added min/max for array size.

@@ -1,4 +1,4 @@
export interface IntegerOptions {
maximum?: number,
minimum?: number,
maxValue?: number,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed these options to be consistent with minItems/minLength. I'm a little torn here because the minimum/maximum naming is what OpenAPI uses, but I actually got a bit confused during this PR and found that using minFoo was clearer.

  • minLength => string length
  • minItems => array size
  • minValue => numeric magnitude

optional?: boolean,
type?: string,
type?: string | Constructor | Constructor[],
Copy link
Contributor Author

@jessemyers jessemyers Jan 21, 2021

Choose a reason for hiding this comment

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

I'm starting to realize the the mixin options should probably not be combined with the recipe options - we currently defined the recipes as accepting MixinOptions & RecipeOptions. I will probably revisit this soon that, for example, you can't pass maxLength (for strings) to the array decorator.

import { Constructor } from '../constructor';

export interface ArrayOptions {
type: Constructor,
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that type is required.

@jessemyers jessemyers merged commit 13e4e65 into main Jan 21, 2021
@jessemyers jessemyers deleted the arrays branch January 21, 2021 20:32
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.

2 participants