Bug 1798871: Fix formik low priority validation issues #4266

rohitkrai03 · 2020-02-10T10:57:46Z

Fixes -

This PR -

Adds a custom hook to trigger formik form validation when a value in formik state changes. This value is specified using value param to the hook.
Uses useDeepCompareMemoize hook to deeply compare values. Also, moves the hook into console/shared
Adds the custom hook call to the components or formik fields that need validation due to use of setFieldValue.

Screencast -

openshift-ci-robot · 2020-02-10T10:57:56Z

@rohitkrai03: This pull request references Bugzilla bug 1798871, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Bug 1798871: Fix formik low priority validation issues

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2020-02-10T12:09:26Z

@rohitkrai03: This pull request references Bugzilla bug 1798871, which is valid.

Details

In response to this:

Bug 1798871: Fix formik low priority validation issues

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

andrewballantyne · 2020-02-10T14:05:22Z

/kind bug

andrewballantyne

Just a couple tiny things.

Question though @rohitkrai03 - #4087 caused this, right? We upgraded to this version to get away from another issue due to an upgrade. And that upgrade, I assume, upgraded rc versions to get away from some issue they didn't want. This is sorta snowballing, no?

What is the real cost of adopting a more stable version? Type issues? Can we override the types or find a TS work around? Doing hacks to make TS work is a lot more comfortable than making hacks to get the JS to work properly - as JS errors can come back to haunt us in the next release. Whereas if we hack TS, we can do proper upgrades / fixes to that in the future without worrying about back ports.

andrewballantyne · 2020-02-11T16:01:45Z

frontend/packages/console-shared/src/hooks/deep-compare-memoize.ts

Suggested change

export const useDeepCompareMemoize = (value) => {

export const useDeepCompareMemoize<T = any> = (value: T): T => {

That way it is clear whatever you're giving is being returned from this method. If they want to type it, they can.

I do see later that this is a move of code, but I'd still like this done.

The above suggestion throws error saying parsing error. I think it should be somthing like

Suggested change

export const useDeepCompareMemoize = (value) => {

export const useDeepCompareMemoize = <T = any>(value: T): T => {

But this has issues with type T not matching with ref.current. Not sure how to correctly type it this way.

Ah sorry, I didn't test it to see the issue. I just made the comment.

React Hooks have generics built into them.

export const useDeepCompareMemoize = <T = any>(value: T): T => { const ref = React.useRef<T>(); if (!_.isEqual(value, ref.current)) { ref.current = value; } return ref.current; };

andrewballantyne · 2020-02-11T16:03:25Z

frontend/packages/console-shared/src/hooks/formik-validation-fix.ts

Suggested change

export const useFormikValidationFix = (value) => {

export const useFormikValidationFix = (value: any) => {

Not a huge fan of this idea, but I would prefer an explicit any instead of an implicit any. We, far too often, forget types. So I'd prefer to see an explicit any rather than nothing.

andrewballantyne · 2020-02-11T16:08:59Z

frontend/packages/dev-console/src/components/import/git/GitSection.tsx

This is fix a temporary solution or a permanent one? Removing functionality like this for our interim hack may be undesirable. I suppose we could just revert this commit if Formik provides an actual solution to this issue.

I've removed use of validateForm from here because it was not doing anything productive anyway. Even after calling validateForm, the validation was running on old stale data which was causing the validation errors. Plus, the custom hook that i've added calls this same method but on the change of data and not right after setFieldValue which is an async action.

I didn't have an issue with the other spots this change happened, because you were using the power of hooks to apply additional functionality additively. This however is you replacing current functionality with this new hook.

Is this hook something we intend to keep using? Or are we only doing this because of typing issues in upgrading. I assume Formik is fixing their own issues and this is not an issue on the latest Formik version, and we are just unable to upgrade due to the dependency cascade issue in the React types.

My only concern here is that we will have to take care to bring back the functionality as it were once we upgrade. We lack the proper safety net (I believe) that comes with unit tests & integration tests to be certain we will be safe with changes.

Latest version of Formik also doesn't fix the issue, Formik team is still in the process of figuring out the solution. So, its not an issue that can be fixed by just upgrading to the latest Formik. And use of validateForm here was redundant and not needed here since every time we call setFieldValue or setFieldTouched, formik also runs the validation. So, once the problem in Formik gets resolved this will automatically start to work without the need of extra call to validateForm or the custom hook. Unless they hange something in the API which will need us to update the code in some way anyways.

Ah I see - I read your comments from top-down on your PR. I apologize for my ignorance :)

rohitkrai03 · 2020-02-11T19:00:46Z

Just a couple tiny things.

Question though @rohitkrai03 - #4087 caused this, right? We upgraded to this version to get away from another issue due to an upgrade. And that upgrade, I assume, upgraded rc versions to get away from some issue they didn't want. This is sorta snowballing, no?

What is the real cost of adopting a more stable version? Type issues? Can we override the types or find a TS work around? Doing hacks to make TS work is a lot more comfortable than making hacks to get the JS to work properly - as JS errors can come back to haunt us in the next release. Whereas if we hack TS, we can do proper upgrades / fixes to that in the future without worrying about back ports.

@andrewballantyne This is not an issue introduced by #4087. This is something that got introduced in Formik 2.0.1-rc2 due to the use of scheduler for state updates. This is the reason I was using rc1 earlier which got bumped to rc5 which broke a lot of our features. Moving to a stable 2.0.3 was a better alternative to fix those broken features. Even the latest stable release of Formik has this validation issue and the formik team is saying this is because of how hooks work. Basically whenever we say setFieldValue, they are sending an async action for updating the formik state but the validation runs on old data since there's no way to know when that the async call to update state completed. There are a lot of issues related to this on the Formik github repo. So, this PR introduces a workaround to fix the issue.

andrewballantyne · 2020-02-11T19:36:22Z

@andrewballantyne This is not an issue introduced by #4087. This is something that got introduced in Formik 2.0.1-rc2 due to the use of scheduler for state updates. This is the reason I was using rc1 earlier which got bumped to rc5 which broke a lot of our features. Moving to a stable 2.0.3 was a better alternative to fix those broken features. Even the latest stable release of Formik has this validation issue and the formik team is saying this is because of how hooks work. Basically whenever we say setFieldValue, they are sending an async action for updating the formik state but the validation runs on old data since there's no way to know when that the async call to update state completed. There are a lot of issues related to this on the Formik github repo. So, this PR introduces a workaround to fix the issue.

As long as we are not taking duct tape and wrapping it around our Formik implementation and calling it a real fix. If this is something that Formik is having issues with then we sorta have no choice - we cannot upgrade out of it.

I just want whatever we do to be a sound solution and not something that will cause us endless back-ports and "critical" 11th hour fixes to make a release go out the door.

andrewballantyne

/lgtm

Changes look good to me now. Thanks Rohit.

christianvogt · 2020-02-11T20:28:43Z

/approve

openshift-ci-robot · 2020-02-11T20:29:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, christianvogt, rohitkrai03

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [christianvogt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2020-02-11T20:31:12Z

/retest