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

[jschuur]

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"

[vikasvmads]

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

[dcousens]

@vikasvmads contributions are very welcome! :blue_heart:

[siahgoodwork]

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.

[dcousens]

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

[siahgoodwork]

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)

[dcousens]

@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:

[siahgoodwork]

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. `
      );
    }

[dcousens]

@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

[siahgoodwork]

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?

[kennedybaird]

@dcousens - is there a reason validation.isRequired is not part of common configuration?

It seems like it could be (maybe with some special handling for virtual fields), if so, it would mean we could have a single assert in field-assertions.ts to solve for all fields.

Otherwise, for single fields, it's reasonably simple - text/index.ts example (after L#85):

    if(isRequired && config.ui?.createView?.fieldMode === 'hidden' && !defaultValue_) {
      throw new Error(`${meta.listKey}.${meta.fieldKey} requires a value, but is hidden on create. Either add a default value or change the create view field mode.`)
    }

Options:

1. Add a shared helper function in root fields folder, and add to each field at the start of the return (meta) =>
2. Make isRequired part of common config, adding a generic assertIsRequiredFieldsHiddenOnCreateHaveDefaultValue to field-assertions.ts.

[dcousens]

@kennedybaird I don't think I can answer that question with any currency, as I think the field parameter types were determined as a result of progressive iteration and development.

What I can say, is that I completely agree with your perspective, but that the addition of that to the common field type will likely be a breaking change. Maybe you can play with the idea in a draft pull request?

[kennedybaird]

Thinking this through a little before diving into code - a really simple solution could be making isRequired a top-level flag.

Then we could mark validation.isRequired as deprecated, and prioritise isRequired?

[dcousens]

@kennedybaird an alternative point is the conflicting nature of db.isNullable, graphql.omit.*, graphql.isNonNull.* and validation.isRequired [and their effect on ui.*].

An advanced developer can probably leverage each of these effectively, but, it's a truth table nightmare to know exactly what is happening.

Replacing validation.isRequired with required should probably better this, otherwise we're only renaming.

[kennedybaird]

I was thinking more of a drop-in replacement with no change in behaviour, "making" was bad wording.

If we added a new required flag, deprecated validation.isRequired, then everywhere that validation.isRequired is used - we check for required first.

And on list / field initialisation, we can make sure only one or the other is used? Now I've re-read your message I'm worried I'm missing something simple however :sweat_smile:

[dcousens]

@kennedybaird I think opting into one or the other is an ideal, the spattering of each is nonsense - but I think the feature warrants planning.

Maybe we can capture the current set of options and outcomes somehow, then opt for the subset that would be defaulted/set by isRequired: true.