form icon indicating copy to clipboard operation
form copied to clipboard

feat: set field errors from the form validators

Open fulopkovacs opened this issue 10 months ago β€’ 10 comments

This PR adds the ability to set field errors from the form validators.

The API would look like this (this is a value returned by a validator):

{
  form: "General form error", // optional,
  fields: {
    username: "Username taken",
    password: "Password is too short"
  }
}

We'll add support for Zod, Yup, and Valibot in a separate PR.

TODO

  • [x] Initial implementation of the feature
  • [x] Add tests for
    • [x] basic fields
    • [x] array fields
    • [x] nested fields in array fields
    • [x] linked fields
  • [x] Clean up the code
    • [x] form-core
    • [x] tests
  • [x] Docs
  • [x] Rebase to main

fulopkovacs avatar Mar 27 '24 22:03 fulopkovacs

☁️ Nx Cloud Report

CI is running/has finished running commands for commit a43feb628cb8fc0b53414b31fd928ba4b0eec8ba. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

πŸ“‚ See all runs for this CI Pipeline Execution


βœ… Successfully ran 2 targets

Sent with πŸ’Œ from NxCloud.

nx-cloud[bot] avatar Mar 27 '24 22:03 nx-cloud[bot]

Codecov Report

Attention: Patch coverage is 97.46835% with 2 lines in your changes missing coverage. Please review.

Project coverage is 89.90%. Comparing base (5473bb8) to head (a43feb6). Report is 98 commits behind head on main.

Files Patch % Lines
packages/form-core/src/FormApi.ts 96.15% 2 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #656      +/-   ##
==========================================
- Coverage   91.55%   89.90%   -1.65%     
==========================================
  Files          21       26       +5     
  Lines         900      981      +81     
  Branches      206      229      +23     
==========================================
+ Hits          824      882      +58     
- Misses         71       93      +22     
- Partials        5        6       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Mar 31 '24 15:03 codecov-commenter

Hi. When are you planning to release it? Thanks.

denisorehovsky avatar Apr 11 '24 03:04 denisorehovsky

Hi. When are you planning to release it? Thanks.

I'm planning to pick this up during the weekend. πŸ˜… Unfortunately there are some urgent things that I have to take care of in the tanstack/query repo (a lot of broken links in the docs), but I'll jump on this PR after that.

fulopkovacs avatar Apr 11 '24 12:04 fulopkovacs

@fulopkovacs Perfect. Thank you. This is indeed necessary. I migrated from react-hook-form, which had a setError function that allowed setting errors when the backend responded with errors. It was surprising that this library doesn't yet support something similar. I think it's a very common use case to have backend validation and then set field-level validation errors.

denisorehovsky avatar Apr 12 '24 02:04 denisorehovsky

Sad status update: I ran into some issues as I started adding more complex tests. This is a bummer because I wanted to get this one ready during the weekend. 😒

fulopkovacs avatar Apr 15 '24 12:04 fulopkovacs

@fulopkovacs It's ok :) Will be waiting for the update

denisorehovsky avatar Apr 15 '24 13:04 denisorehovsky

@fulopkovacs Hey :) You're probably busy, so I'm sorry to bother you, but do you know when you will be able to get back to this PR? Without this solution, I will likely need to find a workaround for properly handling backend errors, or switch back to react-hook-form

denisorehovsky avatar Apr 19 '24 15:04 denisorehovsky

Hey @denisorehovsky, just as a heads up - we don't follow any kind of release schedule or cadence. As we're all volunteer based, we're only able to take on tasks as we have time. Maybe you could consider sponsoring the project financially or making a PR if you're needing priority support? πŸ˜„

Moreover, though, I'm not sure I understand how this is a blocker for anything. This PR is a nicety, not a required feature to merge form and field errors. You can always do that manually by using a helper component.

crutchcorn avatar Apr 19 '24 15:04 crutchcorn

@fulopkovacs Hey :) You're probably busy, so I'm sorry to bother you, but do you know when you will be able to get back to this PR? Without this solution, I will likely need to find a workaround for properly handling backend errors, or switch back to react-hook-form

Hey ☺️,

I'm actively working on this PR! The process is slow, because there are many scenarios to handle. All the form-core tests are passing for the sync validators, and locally I am getting closer to solving the problem of the async validators too, but unexpected things keep popping up (right now I'm solving some issues for the linked fields).

After all the form-core tests pass, I'm planning to add some library tests (React), to make sure that everything is OK.

Then, the PR will go to review, but unfortunately, I don't expect that to be very quick (it's gonna be a beefy PR), and there's always the chance that the approach I chose doesn't fit the project well enough. 😬

In short, I'm sorry, but this will likely take some time. (Honestly, it's more complex than I expected it to be, I thought I'd be ready by the end of last week.)

Finding a workaround is a more realistic solution than waiting for this feature to be merged. For example, you can handle the form's validation on the server with the form's onSubmitAsync validator, and save the field-related errors to your component's local state before you return an error (that you don't have to display btw) for the form. There are also other ways to solve this, I hope you find one that fits your use case.

In situations like this, I'm really sad that it's not my full-time job, it'd be awesome to respond to the users' needs more quickly! 😒 Thanks for your understanding though.

Edit: The tests are finally passing, but there are still some smaller tasks I need to do before the review. Check the PR's updated description for more details!

fulopkovacs avatar Apr 19 '24 16:04 fulopkovacs

Hi all, this feature would be amazing and is very critical, I'll be happy to help but I cannot see what's blocking its merge.

MikeBurkeMed avatar Jul 23 '24 15:07 MikeBurkeMed

Hi all, this feature would be amazing and is very critical, I'll be happy to help but I cannot see what's blocking its merge.

Hey πŸ‘‹! It's waiting for a rebase (by me) before others in the team can review it. Unfortunately too many things have been changed, so it doesn't make sense to change the order of these things.

I was away from the keyboard for 2 weeks, and I won't be able to work on this during the workweek, but I'll look into it on the weekend!

(Can't promise anything though. A lot of PRs have been merged since I've finished this one. I hope it'll go smooth! 🀞)

fulopkovacs avatar Jul 29 '24 16:07 fulopkovacs

I was away from the keyboard for 2 weeks, and I won't be able to work on this during the workweek, but I'll look into it on the weekend!

(Can't promise anything though. A lot of PRs have been merged since I've finished this one. I hope it'll go smooth! 🀞)

Hey! That's excellent, thank you for all your work! Please let me know if I can be of any help.

MikeBurkeMed avatar Aug 01 '24 23:08 MikeBurkeMed

Here's quick report on the progress I made during the weekend:

  • there were a lot of conflicts during the rebase (basically each commit had at least 1 conflict)
  • I decided to create a new branch for myself where all the commits of this branch is squashed into a single one -> I'll work on this new branch for now, and will probably force push the commits from the new branch to this one once the issues caused by the rebase are resolved
  • I'm done resolving the merge conflicts on the new branch, but some tests are failing, so I'll have to fix them (I'll do it next week)
    • only 3 tests failed in form/core, which is a good sign
    • here are the error logs: https://cloud.nx.app/runs/GzTGGOR3Md
image

fulopkovacs avatar Aug 04 '24 21:08 fulopkovacs

commit: a43feb6

@tanstack/angular-form

pnpm add https://pkg.pr.new/@tanstack/angular-form@656
@tanstack/form-core

pnpm add https://pkg.pr.new/@tanstack/form-core@656
@tanstack/lit-form

pnpm add https://pkg.pr.new/@tanstack/lit-form@656
@tanstack/react-form

pnpm add https://pkg.pr.new/@tanstack/react-form@656
@tanstack/solid-form

pnpm add https://pkg.pr.new/@tanstack/solid-form@656
@tanstack/valibot-form-adapter

pnpm add https://pkg.pr.new/@tanstack/valibot-form-adapter@656
@tanstack/vue-form

pnpm add https://pkg.pr.new/@tanstack/vue-form@656
@tanstack/yup-form-adapter

pnpm add https://pkg.pr.new/@tanstack/yup-form-adapter@656
@tanstack/zod-form-adapter

pnpm add https://pkg.pr.new/@tanstack/zod-form-adapter@656

Open in Stackblitz

More templates

pkg-pr-new[bot] avatar Aug 11 '24 10:08 pkg-pr-new[bot]

Current status of this PR: we're gonna have one more review by @crutchcorn. (I introduced some serious changes in the validation, so the more people see it before the merge the better.)

fulopkovacs avatar Aug 22 '24 18:08 fulopkovacs

Just thinking out loud here:

image

This introduces a new behavior that many might consider a "bug". This behavior would occur when the following pattern emerges in a codebase:

export default function App() {
  const form = useForm({
    defaultValues: {
      age: 0,
    },
    validators: {
      onChange: ({ value }) => {
        return {
          fields: {
            age: value.age < 12 ? 'Too young!' : undefined,
          },
        }
      },
    },
  })

  return (
    <div>
      <h1>Field Errors From The Form's validators Example</h1>
      <form
        onSubmit={(e) => {
          e.preventDefault()
          e.stopPropagation()
          void form.handleSubmit()
        }}
      >
        <form.Field
          name="age"
          validators={{
            onChange: ({ value }) => value % 2 === 0 ? 'Must be odd!' : undefined,
          }}
          children={(field) => (
            <div>
              <label htmlFor={field.name}>Age:</label>
              <input
                id={field.name}
                name={field.name}
                value={field.state.value}
                type="number"
                onChange={(e) => field.handleChange(e.target.valueAsNumber)}
              />
              {field.state.meta.errors.length > 0 ? (
                <em role="alert">{field.state.meta.errors.join(', ')}</em>
              ) : null}
            </div>
          )}
        />
        <form.Subscribe
          selector={(state) => [state.canSubmit, state.isSubmitting]}
          children={([canSubmit, isSubmitting]) => (
            <button type="submit" disabled={!canSubmit}>
              {isSubmitting ? '...' : 'Submit'}
            </button>
          )}
        />
      </form>
    </div>
  )
}

Where the developer might want to get two different errors at the same time, but is only reported one of the errors due to how we have this edgecase handled in our code.

Now, to be clear, I don't think this is actually a huge problem - but:

  1. It does play into the idea that for 2.x (depending on how things shake out) we might want to migrate things to an array for errorMap.onChange). [I'm still not fully convinced of this yet]

  2. We should 100% document this behavior before it goes live to avoid confusion


Continuing my review otherwise ignoring this

crutchcorn avatar Aug 27 '24 14:08 crutchcorn