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

Adds {field}.hooks.validate.[create|update|delete] hooks #9057

Merged
merged 12 commits into from
Mar 25, 2024

Conversation

dcousens
Copy link
Member

@dcousens dcousens commented Mar 7, 2024

Similar to #9056

This pull request needed to adopt a slightly different approach, since we can't reasonably change the types of field hooks when field types are part of our public API, and they are not statically determined by nature as the field types themselves return functions, not an object like list.

This is only a slight inconvenience though, since we can throw a runtime error to prevent incompatibilities at the point of creating the GraphQL resolvers (parseFieldHooks). I do want to flatten this in the future, but, for optimistic usecases you could start using .validation.* field hooks now. This won't be compatible with many of the native field types, but that compatibility can come later.

Additionally, as part of this pull request (and part of the validateInput hooks), the text now accepts a default value type of null. This was mentioned as a pain point in #7158 (reply in thread), but has remained unresolved for some time.

If db.isNullable: false, and you pass defaultValue: null, you will end up with a Prisma error if your text input is undefined (this behavior is typical of other fields). If db.isNullable: true, then defaultValue: null will be exactly that.

// | {
// create?: ResolveInputFieldHook<ListTypeInfo, 'create', FieldKey>
// update?: ResolveInputFieldHook<ListTypeInfo, 'update', FieldKey>
// }
Copy link
Member Author

@dcousens dcousens Mar 7, 2024

Choose a reason for hiding this comment

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

We cannot extend the type without it being a breaking change

Copy link

codesandbox-ci bot commented Mar 7, 2024

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 26d3772:

Sandbox Source
@keystone-6/sandbox Configuration

@dcousens dcousens force-pushed the field-hooks-tests branch 3 times, most recently from 32d187a to 6bf7d86 Compare March 25, 2024 06:03
if ((config as any).isIndexed === 'unique') {
throw Error("isIndexed: 'unique' is not a supported option for field type checkbox")
throw TypeError("isIndexed: 'unique' is not a supported option for field type checkbox")
Copy link
Member Author

@dcousens dcousens Mar 25, 2024

Choose a reason for hiding this comment

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

This is a type error, and I think we should remove this in the future

@dcousens
Copy link
Member Author

dcousens commented Mar 25, 2024

I think the distinctions between our nulls is poorly defined. We have too many nulls.

  • GraphQL is String | String!, throws type errors
  • Hooks for validation when isRequired: true, throws validation errors
  • Prisma String? | String, throws prisma errors

Arguably we can unify some parts of this, or restructure it for progressive enhancement with less "conflicting" configuration.
At the least, validation: { isRequired: true } is unhelpful when you can use GraphQL to ensure the input is required.
We should leverage that instead.

Leaves some questions around context.db, but, that has many questions already... anyway.
This pull request doesn't change any of that, but, it is becoming increasingly obvious to me that we should do something different for that.

@@ -24,7 +27,7 @@ export type TextFieldConfig<ListTypeInfo extends BaseListTypeInfo> =
match?: { regex: RegExp, explanation?: string }
length?: { min?: number, max?: number }
}
defaultValue?: string
defaultValue?: string | null
Copy link
Member Author

@dcousens dcousens Mar 25, 2024

Choose a reason for hiding this comment

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

Unlike other fields, the ambiguity of ?: representing '' or null is why this needs to exist.
We should allow null as a defaultValue typically, and maybe check it at build time too.

@dcousens dcousens merged commit 1b55b41 into main Mar 25, 2024
43 checks passed
@dcousens dcousens deleted the field-hooks-tests branch March 25, 2024 09:42
@dcousens dcousens mentioned this pull request Mar 28, 2024
@gautamsi gautamsi mentioned this pull request Jul 8, 2024
5 tasks
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.

1 participant