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

feat(qwikcity/actions): dotnotation field-errors #6568

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

tzdesign
Copy link
Contributor

@tzdesign tzdesign commented Jun 18, 2024

Overview

On very complex nested types, you loose the location or the field the error belongs to.

BREAKING CHANGE:

The fieldErrors type on the action changed from Record<string,string[]>
(simplified) to Record<string,string | string[]> where the key is now dot notation

Closes #5463

More context

@ulic75 added the possibility for complex form with the PR #4634 and we were trying to use it in our cart. As you might imagine, the cart type is very complex and has more than one level. Example action with comparable type here:

export const useAddressFormAction = routeAction$(
  () => {},
  zod$((z) =>
    z.object({
      email: z.string(),
      address: z.object({
        street: z.string(),
        city: z.string(),
        zip: z.string(),
      }),
    })
  )
);

export default component$(() => {
  const addressAction = useAddressFormAction();
  return (
    <Form action={addressAction}>
      <input
        type="email"
        name="email"
        class={{
          error: addressAction.value?.fieldErrors?.email?.length ?? 0 > 0,
        }}
      />
      <br />
      <input
        type="text"
        name="address.street"
        class={{
          error: addressAction.value?.fieldErrors?.['address']?.length ?? 0 > 0,
        }}
      />
      <br />
      <input
        type="text"
        name="address.city"
        class={{
          error: addressAction.value?.fieldErrors?.['address']?.length ?? 0 > 0,
        }}
      />
      <br />
      <input
        type="text"
        name="address.zip"
        class={{
          error: addressAction.value?.fieldErrors?.['address']?.length ?? 0 > 0,
        }}
      />
      <br />

      <button>Send</button>
    </Form>
  );
});

As you can see. You can't determine if address.street is failing exactly, as the errors on address are just an array of strings.

With my approach the errors are the following type:

type FieldErrors = Partial<
    Record<
      | "email"
      | "address.street"
      | "address.city"
      | "address.zip",
      string
    >
  >

With this in mind, the name of the input and the field in fieldErrors will be exactly the same.
And what's impossible now, becomes possible:

<input
        type="email"
        name="email"
        class={{
          error: addressAction.value?.fieldErrors?.email
        }}
      />
      <br />
      <input
        type="text"
        name="address.street"
        class={{
          error: addressAction.value?.fieldErrors?.['address.street'] !== undefined,
        }}
      />
      <br />
      <input
        type="text"
        name="address.city"
        class={{
          error: addressAction.value?.fieldErrors?.['address.city'] !== undefined,
        }}
      />
      <br />
      <input
        type="text"
        name="address.zip"
        class={{
          error: addressAction.value?.fieldErrors?.['address.zip'] !== undefined,
        }}
      />

Arrays

Since @ulic75 also made something like person.0.name possible, I had to change the type for fieldErrors to string | string[]. This means if you have arrays in your validation there are two things which can happen.

The array is missing in the provided data:

fieldErrors will have the property arrayName[] with an array as type.

Example output:

{
   "arrayName[]": ["Required"]
}

The array is NOT missing, but data does not validate:

For example you have addresses like {street: string, zip: number}[] as validation type. You pass address street, but forgot about zip. You will get this:

{
   "addresses[].zip": ["Required"]
}

What is it?

  • Feature / enhancement
  • Bug
  • Docs / tests / types / typos

Checklist:

  • My code follows the developer guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • Added new tests to cover the fix / functionality

Tobias Zimmermann added 4 commits June 18, 2024 08:50
On very complex nested types, you loose the location or the field the error belongs to. Closes QwikDev#5463

BREAKING CHANGE: The fieldErrors type on the action changed from Record<string,string[]>
(simplified) to Record<string,string> where the key is now dot notation

closes QwikDev#5463
Copy link

netlify bot commented Jun 18, 2024

👷 Deploy request for qwik-insights pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 71a1216

@tzdesign
Copy link
Contributor Author

tzdesign commented Jun 18, 2024

@gioboa this is my idea of fixing #5463

Let me know what you think. Added the infos to the docs.
As @mhevery and @wmertens was mention in #5463. Maybe you have an opinion on that?

We are now in production without passing validation and using this flattenZodMap locally in the action body.
Not perfect, but with this pr, we could refactor passing validation again.

Did a draft, as I had issues linking this. Some dirty task for spa_init. Not sure if I am the cause.

Copy link

pkg-pr-new bot commented Jun 18, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

commit: 8ea9715

@builder.io/qwik

npm i https://pkg.pr.new/@builder.io/qwik@6568

@builder.io/qwik-city

npm i https://pkg.pr.new/@builder.io/qwik-city@6568

eslint-plugin-qwik

npm i https://pkg.pr.new/eslint-plugin-qwik@6568

create-qwik

npm i https://pkg.pr.new/create-qwik@6568

@tzdesign tzdesign marked this pull request as ready for review June 19, 2024 06:34
@tzdesign tzdesign requested review from a team as code owners June 19, 2024 06:34
@tzdesign
Copy link
Contributor Author

tzdesign commented Jun 20, 2024

@gioboa I added the array field.

I was reading the code and thought about arrays in general.

My change is helping a lot with deep nested objects, but it's the same kind of unhelpful with complex forms having arrays with objects and arrays.

I guess we could make it even more complex, but I don't think it's necessary for 99% of the people out there.

image

@gioboa
Copy link
Member

gioboa commented Jun 20, 2024

Thanks @tzdesign 🚀 for this great piece of code

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.

[✨] complex form should not flatten errors
3 participants