keystonejs / keystone

The superpowered headless CMS for Node.js — built with GraphQL and React
https://keystonejs.com
MIT License
9.18k stars 1.15k forks source link

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

Closed dcousens closed 6 months ago

dcousens commented 7 months ago

Similar to https://github.com/keystonejs/keystone/pull/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 https://github.com/keystonejs/keystone/discussions/7158#discussioncomment-7584310, 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.

codesandbox-ci[bot] commented 7 months ago

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

Sandbox Source
@keystone-6/sandbox Configuration
dcousens commented 6 months ago

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

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.