-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Update built-in fields to new validate hook syntax #9166
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
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit ea10e02:
|
|
Amazing @acburdine, this is tedious and really appreciated work! 💚 |
|
we can do that, i guess the other one would be part of v-next branch and might take longer, I agree with you to get this going. |
4d56c41 to
2d2b8fc
Compare
d6cbbcc to
2cf6c8c
Compare
| expect(result.errors![0].message).toMatchInlineSnapshot(` | ||
| "You provided invalid data for this operation. | ||
| - Test.price: Price must be greater than or equal to -300" | ||
| - Test.price: value must be greater than or equal to -300" |
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.
Errors are already prefixed with {listKey}.{fieldKey}, this humanized part is redundant and is difficult to understand for names with a few words
| min > max | ||
| ) { | ||
| throw new Error(`${meta.listKey}.${meta.fieldKey} specifies a validation.max that is less than the validation.min, and therefore has no valid options`) | ||
| } |
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.
I used this opportunity to unify each of the numerical fields to share nearly the same code for min and max validation.
| delete: merge(builtinValidate?.delete, hooksValidate?.delete) | ||
| } : undefined, | ||
| } satisfies FieldHooks<ListTypeInfo> | ||
| } |
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.
@acburdine I tidied this up a little to re-use a new merge function, but fundamentally it's the same
| export const typeFunction = text | ||
| export const exampleValue = () => 'foo' | ||
| export const exampleValue2 = () => 'bar' | ||
| export const nonNullableDefault = true |
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.
By adding this for multiselect, and text supporting null in #9057, text is now less special in our tests
| testField: mod.typeFunction({ | ||
| ...(mod.nonNullableDefault ? { | ||
| db: { | ||
| isNullable: true |
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.
These tests had been previously written assuming that db.isNullable is true by default, with an inline exception for text
|
Thanks for starting this work @acburdine, this has been a heavy lift that has been required for a while - I don't think I could have jumped in to update the fields to be aligned with Thank you 💛 |
closes #9165