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

Next #276

Merged
merged 30 commits into from
Feb 1, 2018
Merged

Next #276

merged 30 commits into from
Feb 1, 2018

Conversation

jaredpalmer
Copy link
Owner

@jaredpalmer jaredpalmer commented Nov 28, 2017

  • Sync field-level validations + Docs
  • Async field-level validations + Docs .
  • Document nested path syntax
  • Nested path syntax for deep updates (from Added nested values proposal prototype (#202) #207)
  • <FieldArray> (for now this is bundled with core), might move to formik-array pkg
    • push
    • pop
    • remove
    • insert
    • move
    • swap
    • unshift
  • Document <FieldArray>
  • Upgrade to TS 2.6.2
  • Upgrade to React 16, Jest 21, Enzyme 3
  • Typechecking in test files during development

@LinusU
Copy link

LinusU commented Dec 5, 2017

I think field level validations seems awesome, I'm going to try it out on a form which adds fields dynamically which have been a huge pain currently for me 🍻

src/Field.tsx Outdated
);
} else {
// Otherwise set the error
setFieldError(maybePromise);
Copy link

Choose a reason for hiding this comment

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

Shouldn't this be setFieldError(name, maybePromise)? I think that this is causing an error for me...

Copy link

Choose a reason for hiding this comment

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

Yeah, adding name here fixes this for me. The debugger also told me that setDeep was being called with my error message as the path so this seems to be the culprit. I opened #290 to fix it

@jaredpalmer
Copy link
Owner Author

Will push this when tests pass

@LinusU
Copy link

LinusU commented Dec 5, 2017

Thank you!

(tests seems to be passing now 😃)

@LinusU
Copy link

LinusU commented Dec 6, 2017

So, one thing to think about, which I'm not sure really how to solve, is that setting a validator on field will not rerun that validation if setFieldValue is used somewhere else 🤔

@jaredpalmer
Copy link
Owner Author

True true. Hmmmmm

jaredpalmer and others added 5 commits December 9, 2017 19:21
* #283 Update to React 16, TypeScript 2.6.2

* Update Travis build to use npm instead of yarn

* Move back to yarn (#301)

* Fix travis script to use yarn.
@brikou
Copy link
Contributor

brikou commented Dec 12, 2017

@jaredpalmer this looks great!

@brikou
Copy link
Contributor

brikou commented Dec 12, 2017

@jaredpalmer it looks like when dealing nested array the required name should look like my_array.0.my_obj.my_prop. Yup validation uses instead my_array[0].my_obj.my_prop in default errors. I you follow this convention then yup validation would work out of the box, WDYT?

@jaredpalmer
Copy link
Owner Author

Yeah we need to add bracket support to dlv

};
const { render } = this.props;
return render
? (render as any)({ ...arrayHelpers, form: this.context.formik })
Copy link
Contributor

Choose a reason for hiding this comment

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

form is missing in ts definition

Copy link
Owner Author

Choose a reason for hiding this comment

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

ahh yes good looks

Copy link
Contributor

Choose a reason for hiding this comment

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

As you fix missing def, can you release a new alpha, as I'm living on the edge :D? Thank you so much!

Copy link
Contributor

Choose a reason for hiding this comment

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

@jaredpalmer thanks 👍

@@ -639,7 +619,7 @@ function warnAboutMissingIdentifier({
* Transform Yup ValidationError to a more usable object
*/
export function yupToFormErrors<Values>(yupError: any): FormikErrors<Values> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yah will do

jaredpalmer and others added 5 commits December 29, 2017 10:35
* Fix FieldArray component prop

* nit: test names [skip ci]
* feat(Field): Allow bracket path support

* chore(deps): Add types for lodash.topath

* chore(deps): yarn.lock
@renchap
Copy link

renchap commented Jan 18, 2018

I just implemented an custom password input field with built-in strenght check and validation using the field-level validation and it works amazingly well, great work!

Have you considered the interaction between those field-level validations and the validationSchema prop? Currently you can not use both at the same time as errors are reset in validateYupSchema (https://github.com/jaredpalmer/formik/blob/next/src/formik.tsx#L343).
I think it would be better to merge errors from the fields and the one from the schema, but I am not sure on how this could work. Any ideas?

@renchap
Copy link

renchap commented Jan 19, 2018

To add more details to my previous comment:
I have a validationSchema to ensure that email is required and a field-level validation on the password field.

When I add calls to console.log in my form render function, I see that Formik's errors is changed twice when I make a change to a field:

  • First update: {email: "email is a required field", password: "Password too weak"}
  • Second update (immediatly after): {email: "email is a required field"}

After some debugging, it seems related to runValidationSchema. When there are no schema errors, the errors are reset:

formik/src/formik.tsx

Lines 341 to 343 in 560a5e3

validateYupSchema(values, schema).then(
() => {
this.setState({ errors: {} });

When there are schema errors, the errors are replaced by the one from the schema validation:

formik/src/formik.tsx

Lines 348 to 350 in 560a5e3

(err: any) =>
this.setState({ errors: yupToFormErrors(err), isSubmitting: false })
);

They should be merged with errors coming from the fields (and eventually overriding them, depending on which one you want to prioritize).
I will try to write my own validation function based on runValidationSchema and Yup to see if it works.

@renchap
Copy link

renchap commented Jan 19, 2018

And a related error, probably masked by the behaviour I described above previously.
Starting from a state.errors = { password: "invalid" }, calling setFieldError('password', undefined) results in state.errors = { password: undefined }.
A way to fix it would be to call delete on the property in setDeep (after cloning the object to not mutate the argument).

Please tell me if you prefer separate issues about these reports.

* Fix #280. Change the meaning of dirty and isValid

* Lock in to path dep

* Update tests for new definition of dirty

* Update docs
@bogdansoare
Copy link

Merging the errors from field-level validations and validationSchema would be really useful

@renchap
Copy link

renchap commented Jan 23, 2018

I digged a bit more into this, and the real issue is merging validation results when some (or all) of them are async (either from validationSchema, a validation function, or field-level validations).

I do not really know how to fix this properly, as you need to know when to merge (the new validation results that come in needs to be applied on top of the existing ones). Probably something like:

  • when we need to validate the form, reset the validation results, generate a "validation pass id"
  • when an async validation resolves, if its for the current "validation pass id", merge it into the existing results. Otherwise ignore it.

This is needed as we can have a new validation pass happening while some async results did not resolve yet (like if coming from a slow server-side process). As there is no way to cancel promises, we need to check if the results are current when it resolves.

I might have missed something here, I am still trying to grasp the whole codebase.

@jaredpalmer
Copy link
Owner Author

Okay folks, going to merge and cut a release.

@jaredpalmer jaredpalmer merged commit b34fe0a into master Feb 1, 2018
@LinusU
Copy link

LinusU commented Feb 1, 2018

Great work 🎂

@jaredpalmer jaredpalmer deleted the next branch May 29, 2019 20:10
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.

7 participants