jotaijs / jotai-form

Form atoms for Jotai
MIT License
135 stars 1 forks source link

Form-level validation #3

Closed dai-shi closed 2 years ago

dai-shi commented 2 years ago

atomWithValidation only supports field-level validation.

For example, suppose pricePerItem field is min(1).max(10), and numberOfItems is min(0).max(20). We may want total price pricePerItem * numberOfItems to be min(0).max(100).

It would be nice to have form-level validation.

barelyhuman commented 2 years ago

so, a way to derive validation schema for certain types of data. To start off we could do numbers

and in case of multiplication and addition at least the min and max can be calculated by multiplying / adding the min/max of each individual schema

But didn't we plan not to abstract the validation schema and let the user do it?

Would something like this make sense?

const fieldAtom = atomWithValidate(0, {
  validate: async (v) => {
    //  validation logic
  },
});

const field2Atom = atomWithValidate(0, {
  validate: async (v) => {
    //  validation logic
  },
});

const formSchema = Yup.number().min(0).max(200);

const formAtom = atomWithValidate(0, {
  validate: (v, get) => {
    formSchema.validate(get(fieldAtom).value * get(field2Atom).value);
  },
});
dai-shi commented 2 years ago

But didn't we plan not to abstract the validation schema and let the user do it?

Yeah, I'd follow atomic model as much as possible. And, let users do it. But, if there were a good pattern to cover, why not. (I hope form-level validation to be optional, not mandatory.) So generally, it's still open for discussions.

Would something like this make sense?

I see. DX-wise, it seems a bit weird. Maybe we should work on "form submission" feature before "form-level validation".

barelyhuman commented 2 years ago

On that, so let's say I write a simple login page, something like this https://codesandbox.io/s/zealous-lalande-blfbzo?file=/src/App.js

where the useJotaiForm actually comes from the library (if we are able to solve the next few issues)

  1. The component using the useJotaiForm hook re-renders twice right now , aka, it renders 1 time for each atom in the hook.
  2. The above doesn't consider the initial mount which well would occur even if didn't use the hook so this can be ignored for now
  3. Is it possible for us to create dynamic atoms inside a hook? I don't think so, I tried but the re-renders went crazy,

if we can get 3 to work then we can create a single form atom that can be both read from(already works) and written to (due to the nature of derived atoms, this isn't possible)

dai-shi commented 2 years ago

where the useJotaiForm actually comes from the library

This would still be open for discussion, but I'd like to make the library pure-jotai, which means it provides only atom I/F. No hooks. atoms are conceptually framework agnostic. (For example, you can assume it works with jotai-jsx if it's only atom I/F.)

The library being framework agnostic isn't super necessary, but it's good mental model to design jotai atomic API.

barelyhuman commented 2 years ago

Yeah, the hook can be avoided, I'm just trying to imagine a way to combine the existing atoms with the atomWithValidate the above is what an implementation might look like

dai-shi commented 2 years ago

Can you show how it'd look like with atom I/F?

barelyhuman commented 2 years ago

It's not pretty in terms of DX, hence the hook approach

import { atom, useAtom } from "jotai";
import { atomWithValidate } from "jotai-form";

const emailFieldAtom = atomWithValidate("", {
  validate: (v) => v
});

const passwordFieldAtom = atomWithValidate("", {
  validate: (v) => v
});

const formAtom = atom(
  (get) => ({
    email: get(emailFieldAtom),
    password: get(passwordFieldAtom)
  }),
  (get, set, newValue) => {
    if (newValue.key === "email") {
      set(emailFieldAtom, newValue.value);
    } else if (newValue.key === "password") {
      set(passwordFieldAtom, newValue.value);
    }
  }
);

export default function App() {
  const [form, setForm] = useAtom(formAtom);

  const handleSubmit = (e) => {
    e.preventDefault();
  };

  console.log("re-render", { form });

  return (
    <div className="App">
      <form onSubmit={handleSubmit}>
        <input
          placeholder="email"
          value={form.email.value}
          onChange={(e) =>
            setForm({
              key: "email",
              value: e.target.value
            })
          }
        />
        <input
          placeholder="password"
          type="password"
          value={form.password.value}
          onChange={(e) =>
            setForm({
              key: "password",
              value: e.target.value
            })
          }
        />
        <button type="submit">Submit</button>
      </form>
    </div>
  );
}
dai-shi commented 2 years ago

I'd want something like this:

const formAtomWithAtoms = (atoms, validate) => {
  return atom(
    (get) => {
      const value = Object.fromEntries(Object.entries(atoms).map(([k, v]) => [k, get(v)]));
      try {
        return {
          value: validate(value),
          isValid: true,
        };
      } catch (error) {
        return {
          value: validate(value),
          error,
        };
      }
    },
    (get, set, arg) => {
      ...
    }
  );
};

const formAtom = formAtomWithValidate(
  {
    email: emailFieldAtom,
    password: passwordFieldAtom,
  },
  validate: ...
);
barelyhuman commented 2 years ago

Well, that's the dynamic variant, but yeah, basically this but I think creating a new object for

return {
 value
 isValid  
}

would be redundant so maybe just the validity part will be from the function and everything else will just be spread from the original atom.

We can do this, and for the setters we are going with the {key:"field", value:"data"} approach or do you have something else in mind? cause atom's don't inherently have any idenfication right?

dai-shi commented 2 years ago

just the validity part will be from the function and everything else will just be spread from the original atom.

good point.

const atomWithCombinedValidate = (atoms, options) => {
  const { validate } = options;
  return atom((get) => {
    const value = ...;
    try {
      validate(value); // hm, we can't use the returned (validated) value
      return { isValid: true };
    } catch (error) {
      return { error };
    }
  });
};

Now, I'm not sure if we can support async validate.

barelyhuman commented 2 years ago

Now, I'm not sure if we can support async validate.

That's an issue, yes. Joi is synchronous and yup has synchronous variants

I'd still want it to be async since I have seen cases where the validations are on the server API (rare but they exist) we'll just have to decide if we want to support it for now

but doesn't atom get work with async? 🤔 I saw an example somewhere or i'm completely wrong

dai-shi commented 2 years ago

The current atomWithValidate supports async and has isValidating prop. See: examples/02 I first wanted to use <Suspense> but it doesn't seem to fit well with forms, so far. atom get works with async, but it's Suspense-based.

barelyhuman commented 2 years ago

In that case, I'll build the above in a branch and a codesanbox build should help up iterate over examples to see the shortcomings before solidifying the approach

barelyhuman commented 2 years ago

https://codesandbox.io/s/festive-goldberg-qgx857?file=/src/App.js

Working version of the above, not optimal in terms of renders. I still have to debug why the multiple renders

dai-shi commented 2 years ago

thanks for the example. how do we add form level validation?

barelyhuman commented 2 years ago

by form level, you mean dependent validation between the atoms? right?

dai-shi commented 2 years ago

yes, for example, you have two text fields and you want to restrict total length of two texts.

barelyhuman commented 2 years ago

https://codesandbox.io/s/festive-goldberg-qgx857?file=/src/atomsToForm.js

Added one of the variations from your preferred ways of getting it done, I tried another one that would help with async but that would make it messy for developer experience

dai-shi commented 2 years ago

Looks good. I might consider not including field errors in form atom, or at least separating form errors from field errors.

We want to care with isValidating from field atoms.

barelyhuman commented 2 years ago

well, that has to be added in both field and form atoms, will get that done sometime today. As for the separation of field and form error, I'll get that done as well

barelyhuman commented 2 years ago

Additional Stuff that needs to be done

Notes for self: API designs for both non-atomic and atomic approaches and if possible a curried function approach to simplify usage. Preferred approach: Atomic Fallback preference: Curried

barelyhuman commented 2 years ago

https://codesandbox.io/s/festive-goldberg-qgx857?file=/src/atomsToForm.js Now has the isValidating on the form level and the field level should already be working because of jotai-form

Let me know if the method of implementation makes sense to you

dai-shi commented 2 years ago

(I'll have a look next week.)

dai-shi commented 2 years ago

https://codesandbox.io/s/festive-goldberg-qgx857?file=/src/App.js

Hi, here are some comments.

barelyhuman commented 2 years ago

the demo doesn't seem working?

Good chance of that happening, added a removed quite a bit to experiment with the async handling

for the api, I still feel like we should include field errors in formData, because we have values and can even set a value

Oh you meant to have the fieldErrors as a separate property and not completely remove them

for the impl, the promise handling is not appropriat. it might work, but we should make read function pure. that's why it's extremely hard to implement async validation

I can now relate, took a bit of removal and addition to even get the base working without adding additional re-renders per change

I'll see what I can do, I have a different approach I wanna try, and let's see if that makes sense

barelyhuman commented 2 years ago

completed in: https://github.com/jotai-labs/jotai-form/pull/5