logaretm / vee-validate

✅ Painless Vue forms
https://vee-validate.logaretm.com/v4
MIT License
10.79k stars 1.26k forks source link

Fields don't clean up their rules properly #4643

Closed bgoscinski closed 8 months ago

bgoscinski commented 9 months ago

What happened?

I created a field using the composition API (useField), and I have a template that conditionally renders a different version of it (a <Field> with the same path, but different validation rules). When I change the condition in such a way that causes the <Field> to be unmounted then vee-validate still respects the <Field>'s rules.

I dug a bit into the vee-validate internals and found out that useField registers onBeforeUnmount that should cleanup the relevant pathState from the form but the removePathState finds and removes a pathState with different id than the one created by <Field>. I think that this line https://github.com/logaretm/vee-validate/blob/b96155c5a558cb3d219e0c8a93b4db36a9bf9f40/packages/vee-validate/src/useForm.ts#L562 should have been written more like this:

const idx = pathStates.value.findIndex(s => {
  return (
    s.path === path &&
    (Array.isArray(s.id) ? s.id.includes(id) : s.id === id)
  )
});

I can prepare a PR with this change if you think this is the right way to fix it

Reproduction steps

  1. Visit https://stackblitz.com/edit/vee-validate-v4-checkboxes-t1tqgz?file=src%2FApp.vue
  2. Notice that form status is "Invalid". This is expected because a <Field> which specifies rules=required is rendered.
  3. Fill in the field, notice that Form status goes to "Valid"
  4. Clear the field, notice that Form status goes back to "Invalid"
  5. Now click the "Toggle field" button. This causes <Field> to be unmounted and should result in removal of the pathState that specifies the required rule.
  6. Notice that the Form status is still "Invalid" even though the only registered field has "always true" validator

Version

Vue.js 3.x and vee-validate 4.x

What browsers are you seeing the problem on?

Relevant log output

No response

Demo link

https://stackblitz.com/edit/vee-validate-v4-checkboxes-t1tqgz?file=src%2FApp.vue

Code of Conduct

bgoscinski commented 9 months ago

In the meantime I created a very hacky workaround:

import { Field, FormContextKey } from 'vee-validate';
import { getCurrentInstance, inject, onBeforeUnmount } from 'vue';

const { setup } = Field;
Field.setup = function wrappedSetup(props, ctx) {
  onBeforeUnmount(() => {
    const form = inject(FormContextKey, undefined);
    if (!form) return;

    const { id } = getCurrentInstance().exposed.meta;
    const states = form.getAllPathStates();
    const idx = states.findIndex(
      (state) => (Array.isArray(state.id) ? state.id.includes(id) : state.id === id),
    );

    if (idx >= 0) {
      // Move the correct pathState to the beginning of the states array
      // so that vee-validate's removePathState finds it first
      states.unshift(states.splice(idx, 1)[0]);
    }
  });

  return setup.call(this, props, ctx);
};

Stackblitz with the workaround applied: https://stackblitz.com/edit/vee-validate-v4-checkboxes-1zts45?file=src%2Fmain.ts

logaretm commented 8 months ago

Thanks for reporting this, I took a quick look and seems like it was caused by a couple of assumptions we had:

  1. Always one field with the path would exist at any given time.
  2. It is unlikely a field that would get registered before one of the same name is mounted.

feel free to PR it. we would need a test to accompany it if possible.