keystone icon indicating copy to clipboard operation
keystone copied to clipboard

Hidden, required fields don't show an error on create

Open jschuur opened this issue 3 years ago • 15 comments

This one had me stumped for a while today. I couldn't create a new entry in a list, because whenever I hit save, nothing happened. No loading animation in the button, no error message, no network activity or console error in the browser.

Eventually, I figured out that I had a field marked as both hidden in a createView but also with isRequired: true. I had set up an auto-generated value for it in a hook, but forgot to remove the required part.

Would have been handy to see an error message about this :)

    slug: text({
      validation: { isRequired: true },
      ui: {
        createView: { fieldMode: 'hidden' },
      },
    }),

Versions:

    "@keystone-6/auth": "^4.0.1"
    "@keystone-6/core": "^2.2.0"
    "@keystone-6/fields-document": "^4.1.0"

jschuur avatar Aug 24 '22 00:08 jschuur

Hi, If the issue still open, I would like to work on it. :)

vikasvmads avatar Sep 07 '22 03:09 vikasvmads

@vikasvmads contributions are very welcome! :blue_heart:

dcousens avatar Sep 07 '22 05:09 dcousens

Would love to work on this. What would be a good approach to address this case?

One way I can think of is to enforce a default value if isRequired:true && createView: {fieldMode: "hidden"} via the type definition.

siahgoodwork avatar Jan 26 '23 13:01 siahgoodwork

@siahgoodwork that sounds sensible, and can easily be enforced with a runtime error, contributions welcome! :blue_heart:

dcousens avatar Jan 26 '23 22:01 dcousens

Sweet thanks. ~Where in the source do I start looking if I want to throw a runtime error?~ (Think I found it - the source of the respective fields)

siahgoodwork avatar Jan 27 '23 01:01 siahgoodwork

@siahgoodwork I think we should be able to generalise this for all fields, not implement it per field. Let me know if you can't find that :pray:

dcousens avatar Jan 27 '23 02:01 dcousens

It seems a TypeScript solution might be over engineering at this point - upon thinking through it looking at the source. (I'm thinking along the lines of modifying CommonFieldConfig to a differentiated union type... probably not the smartest way) However @dcousens if you feel that it's an appropriate solution, I'm game to have a go at it but might need your help to get the right framing first.

For now I managed to throw a runtime error for a specific field (Text field). However per @dcousens 's comment I can't seem to figure out where to implement this generally for all fields, would also appreciate a pointer there!

For reference here's what I implemented that seemed to help warn users if they have this specific configuration:

    if (
      config.ui?.createView?.fieldMode === 'hidden' &&
      _defaultValue === undefined &&
      _validation?.isRequired === true
    ) {
      throw new Error(
        `The text field at ${meta.listKey}.${meta.fieldKey} is a required field, but is set to hidden on create. Therefore a default value is required. `
      );
    }

siahgoodwork avatar Jan 27 '23 02:01 siahgoodwork

@siahgoodwork I will look into this on Monday, but I suspect we should put this kind of error checking in initialiseLists (packages/core/src/lib/core/types-for-lists.ts) - probably at the start of the function.

a TypeScript solution might be over engineering at this point

I think so too

dcousens avatar Jan 27 '23 05:01 dcousens

From types-for-lists.ts I managed to follow the trail to packages/core/src/lib/core/field-assertions.ts - would this be the place to add an assertion for the case we're trying to catch here? Seems quite neat.

So assuming we can add an assertion here, it is straightforward to check the field.ui.createView.fieldMode value

However I have some questions regarding the 2 other values we will need:

  1. validateInput is now a function inside the hook key (upon instantiation?). How do we check, from that function, whether the original schema had isRequired: true ?

  2. Does field.dbField.default map to the defaultValue provided in original schema?

  3. Is it safe to assume that this limitation only applies to scalar dbField types?

siahgoodwork avatar Jan 27 '23 06:01 siahgoodwork