blitz-js / legacy-framework

MIT License
3 stars 2 forks source link

validateZodSchema error messages don't work for nested objects when creating an entity #58

Closed frankiesardo closed 3 years ago

frankiesardo commented 3 years ago

What do you want and why?

When you create a new entity, say Project, you might have something like this:

const CreateProject = z.object({
  name: z.string()
})

<ProjectForm
  schema={CreateProject}
  .../>

function ProjectForm<..> {
  <LabeledTextField name="name" label="Name" placeholder="Name" />
}

This works fine. Fronted validation correctly highlights the LabeledTextField when it's touched and it's empty because validateZodSchema returns {name: "Required"} and React Final Form correctly interprets that and marks the field as invalid.

But if your entity has nested fields, validateZodSchema doesn't work out of the box. Consider this example:

const CreateProject = z.object({
  name: z.object({ nested: z.string() }),
})

<ProjectForm
  schema={CreateProject}
  .../>

function ProjectForm<..> {
  <LabeledTextField name="name.nested" label="Name" placeholder="Name" />
}

When the form input is empty, the form state is {}. When the form validation runs, validateZodSchema(CreateProject)({}) returns {name: "Required"} and there's no way for React Final Form to link this error to the <LabeledTextField name="name.nested"/>

Possible implementation(s)

If we want this to work, validateZodSchema(CreateProject)({}) should return{name: {nested: "Required"}}

Another suggested solution was to initialise the form state using initialValues. Something like:

<ProjectForm
  schema={CreateProject}
  initialValues={{name: {}}}
  .../>

The problem is that this generates a compile-time error I really don't want to suppress: Type {name: {}} is not assignable to CreateProject

Additional context

Repo that reproduces the issue https://github.com/frankiesardo/rff-nested-example

This test suite should be changed https://github.com/blitz-js/blitz/blob/0243df5b6a03798fa6481fd9fe3b5f49c0456601/packages/core/src/utils/index.test.ts#L30 so e.g. validateSchema(NestedSchema, {}) returns {nested: {test: "Required"}}

Workaround

Initialize the form state using initialValues. Something like:

<ProjectForm
  schema={CreateProject}
  initialValues={{name: {}}}
  .../>
flybayer commented 3 years ago

Thank you! Ready for someone to work on.

satya-nutella commented 3 years ago

@flybayer 3 is my lucky number. Can I take this up?

flybayer commented 3 years ago

Yep!

satya-nutella commented 3 years ago

@flybayer I am slightly confused as to what needs to be done here since this is a use case I have not encountered before. Can you help me figure out what needs to be done? I'm not used to monorepos and find myself kind of lost

flybayer commented 3 years ago

@meehawk sure thing.

  1. Add this new test case to https://github.com/blitz-js/blitz/blob/canary/packages/core/src/utils/index.test.ts

    it("formats the nested zod error with empty input", () => {
    const NestedSchema = z.object({
      nested: z.object({
        test: z.string(),
      }),
    })
    
    const result = validateSchema(NestedSchema, {})
    expect(formatZodError(result.error)).toEqual({ nested: {test: "Required"} })
    })
  2. cd into packages/core and then run yarn test and you will see that new test fail.
  3. Change code in https://github.com/blitz-js/blitz/blob/0243df5b6a03798fa6481fd9fe3b5f49c0456601/packages/core/src/utils/index.ts#L21 to make all the tests pass
satya-nutella commented 3 years ago

@flyber I've noticed the function recursiveFormatZodErrors and its implementation. The way zod works, if we do:

Schema = z.object({
  nested: z.object({
    test: z.string()
  })
})

...

Schema.parse({})

The error.format returns:

{ _errors: [], nested: { _errors: [ 'Required' ] } }

I am not sure how do we catch the nested errrors in this case since they are not displayed by path. If however we did

Schema.parse({ nested: {} }

In this case, it shows error on the test field

Do you have any suggestions on this?

flybayer commented 3 years ago

@meehawk hmm good question. I think probably we can manually extract the information from the error object instead of using .format. Can you investigate that? Perhaps look at the source of .format (in zod repo)

satya-nutella commented 3 years ago

@flybayer Sure, I'll try to take a look at the zod internal implementation. I did take a look at the error object itself but it doesn't seem to expose the nested error path

flybayer commented 3 years ago

Maybe @colinhacks can help us? (Colin, we're trying to extract nested errors, so that .parse({}) returns {name: {nested: "Required"}} instead of {name: "Required"}

satya-nutella commented 3 years ago

@flybayer I tried looking at Zods internal implementation. Feel kinda lost but I feel we should get to something until @colinhacks replies

colinhacks commented 3 years ago

There's no way to do this with the schema as written. Once Zod sees that there's no value associated with <input>.nested it doesn't try to validate deeper.

You can handle this with preprocess (new in zod@3.8 feature). Basically if nested is missing, Zod swaps in an empty object before parsing occurs. This is required at every level of nesting.

const Schema = z.object({
  nested: z.preprocess(
    val => val ?? {}, 
    z.object({ test: z.string() })   
  ),
});

Schema.parse({})
/*
{
  "_errors": [],
  "nested": {
    "_errors": [],
    "test": {
      "_errors": [
        "Required"
      ]
    }
  }
}
*/
satya-nutella commented 3 years ago

Aah. @flybayer this seems to have put us on hold. In a way though I see this "fail early" make sense in that we dont go nested if the upper layer fails itself. What are your thoughts?

flybayer commented 3 years ago

Hmm ok. Looks like there's nothing we can do here, so closing. Thanks for looking into it @meehawk!

Workaround

Explicitly provide nested objects to form initialValues:

initialValues={{nested: {}}}
frankiesardo commented 3 years ago

@flybayer if initialValues is the suggested workaround, how do we expect people to strongly type their Form schemas. Example

const Example = z.object({ nested: z.object({ test: z.string() }) });

const ExampleForm = () => {
  return (
    <Form
      submitText="Create Account"
      schema={Example}
      initialValues={{ nested: {} }}
      onSubmit={() => {}}
    />
  );
};

The error is Property 'test' is missing in type '{}' but required in type '{ test: string; }'

Do we suppress this typescript error?

frankiesardo commented 3 years ago

I think even if you set initialValues={{ nested: { test: "" } }} you have a runtime error when you add something to the input field (say "foo") and then you delete the string in the input. FinalForm deletes the entry for {test: "foo"} in its form state and the error is

Step to reproduce:

flybayer commented 3 years ago

@frankiesardo you have the type issue regardless of the error display, right? Error display doesn't affect the types. It's perfectly acceptable to do initialValues={{ nested: {} } as any} in this case.

The Cannot set a non-numeric property on an array error occurs in that sandbox because it is not using validateZodSchema inside Form (need to upgrade blitz and zod versions). You can see it working here: https://codesandbox.io/s/stupefied-raman-sl722?file=/app/pages/index.tsx

frankiesardo commented 3 years ago

Thanks @flybayer will do 👌