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

docs(validation): Improved validation guide #484

Merged
merged 5 commits into from
Oct 31, 2023

Conversation

juliendelort
Copy link
Contributor

Issue: #477

Add more info to the validation guide page.

While working on this, I've identified a few issues and inconsistencies. I'll probably create issues and discussion topics about these, but listing them here first in case I've missed some things:

1. formAPI's onChange not called

formAPI's onChange doesn't seem to be called at all and it expects the same return type as fieldAPI's onChange. If we want to allow doing validation at the form level in onChange, we probably need it to return a mapping of errors per field (same with onBlur and async methods). Here is a repro of this.

2. Validate untouched forms

As I have mentioned in the docs, it seems that the way to prevent submission of untouched forms is to do some validation in the onSubmitAsync prop (at the Field level). However, once the form is in error due to an error raised in onSubmitAsync, it seems the form is stuck in error and the user can't submit, even if they've fixed the issue. Here is a repro. Try submitting the form before making any change. Then it's impossible to submit again, even after providing correct values to the fields.

3. sync vs async validation

I don't know the motivation behind it but I wonder if we actually need to separate sync and async validation? For example, instead of having onChange and onChangeAsync, it could be simpler if we just had onChange that would accept a function that either returns a promise (async) or directly a validation error (sync)?

4. Per-event validation

This is a broader discussion topic but I'm seeing potential issues with separating out validation per event (onChange vs onBlur vs onMount etc...). I don't know if there is a use case where users would want to do different validations in onChange and onBlur and onSubmitAsync for example. From my experience, a field is either valid or invalid at a given time, regardless of when the validation is done, and users may often end up reusing the same validation rules in onChange and onSubmitAsync or in onBlur and onSubmitAsync for example.

Imo, it could make things simpler if we decoupled the validation rules (what makes a field valid) from when the validation should be done.

Another challenge created by this approach is that errors are cleared by the same event that raised them. For example if onBlur returns a validation error, that error will only be cleared the next time the field is blurred. In some use cases, users could want to do the initial validation on blur but remove the errors as the user is typing to fix them. Here is an example: (type 2 characters in the first name field then focus the other field: "First name must be at least 3 characters" is shown. It could be good to be able to remove the error as soon as the user types the third character).

I believe this is also what is causing point 2 above about validating untouched forms.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 21, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@natedunn
Copy link

Thanks for bringing up the issues, especially with #2 being an absolute menace. @crutchcorn should these be broken out to their own issues for visibility or are they ok here?

@juliendelort
Copy link
Contributor Author

This recent issue pretty much covers point 2.

I've opened issues and discussions for the other points:

@crutchcorn
Copy link
Member

@juliendelort love the efforts you've put into documenting this with us all and making things go from @tannerlinsley and my brain to more on-paper. This is huge, sincerely thank you.

Here's my thoughts, and let me know your thoughts;

  • All 4 of the raise items are results of some flavor of bug in the codebase. We've since responded to the discussions, issues, and opened new issues to clarify
  • As such, we shouldn't change our docs based off of bugs (ideally, we should just fix our bugs)

With this in mind, here's how I would expect TanStack Form to behave (just in one place to avoid confusion):

export default function App() {
  const form = useForm({
    defaultValues: {
      firstName: '',
      lastName: '',
    },
    validator: zodValidator,
    // This will be renamed in the future to avoid confusion with `change` DOM event
    // This validation will run on submit also
    onChange: z.object({
      firstName: z.string().min(3),
    }),
    onSubmit: async (values) => {
      console.log(values)
    },
  })

  return (
    <div>
      <form.Provider>
        <form
          onSubmit={(e) => {
            e.preventDefault()
            e.stopPropagation()
            void form.handleSubmit()
          }}
        >
          <div>
            <form.Field
              name="firstName"
              // This validator will run in addition to the form validator
              // In addition, this validation will run on submit also
              onChange={(value) =>
                value.includes('error') && 'No "error" allowed in first name'
              }
              // This validation will run on submit also
              onBlur={z.string().max(100)}
              onChangeAsyncDebounceMs={500}
              // This validation will run on submit also
              // And will run after the debounce time
              onChangeAsync={(value => checkDatabaseForName(value)}
              children={(field) => {
                // ...
              }}
            />
          </div>
          {/* ... */}
        </form>
      </form.Provider>
    </div>
  )
}

Does this help answer any questions that might arise from 1-4 as outlined here?

I'm happy to review the docs if this all sounds good to you @juliendelort - what do you think? :)

@juliendelort
Copy link
Contributor Author

juliendelort commented Oct 28, 2023

Thanks @crutchcorn, sounds good to me and the code snippet helped a lot! I've made a few updates based on that and this is ready for a first round of reviews!

Copy link
Member

@crutchcorn crutchcorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the updates and tone of these new docs. Two thoughts we'll want to fix, then we can merge

docs/guides/validation.md Outdated Show resolved Hide resolved
docs/guides/validation.md Outdated Show resolved Hide resolved
@crutchcorn crutchcorn merged commit fd88d05 into TanStack:main Oct 31, 2023
5 checks passed
@juliendelort
Copy link
Contributor Author

Hey @crutchcorn I don't know if it's related to this being merged but the validation page is not loading for me: https://tanstack.com/form/latest/docs/guides/validation

Screenshot_2023-10-31-07-47-27-77_e4424258c8b8649f6e67d283a50a2cbc.jpg

@crutchcorn
Copy link
Member

Fun!

CC @tannerlinsley

@tannerlinsley
Copy link
Collaborator

I’ll debug this.

@Savageman
Copy link
Contributor

@crutchcorn Sorry for a late and unrelated question to this PR.

I saw this:

          onSubmit={(e) => {
            e.preventDefault()
            e.stopPropagation()
            void form.handleSubmit()
          }}

and I was wondering why the void was here, is it useful?

@crutchcorn
Copy link
Member

@Savageman I'm begging to think we should remove this - we get more questions about this than anything else and it's really not required 🫠

The only reason why it's there at all is that TSESlint warns us when we don't have it there as handleSubmit returns a promise.

If you wanna make a PR to remove it, I'll merge it

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.

5 participants