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

ValidationObserver scoped slot doesn't update dirty/pristine flag #2209

Closed
dmoebius opened this issue Aug 12, 2019 · 9 comments
Closed

ValidationObserver scoped slot doesn't update dirty/pristine flag #2209

dmoebius opened this issue Aug 12, 2019 · 9 comments
Labels
🐛 bug Unintended behavior

Comments

@dmoebius
Copy link

Versions

  • vee-validate: 2.2.14
  • vue: 2.6.10

Describe the bug
Using ValidationObserver with a scoped slot to retrieve the current values of the flags dirty and pristine:

  <validation-observer tag="form" v-slot="{ dirty, pristine, valid, invalid, errors }">

    <validation-provider name="Text" :rules="'required|min:2'"  vid="text">
      <label>Text
        <input v-model="text" type="text"/>
      </label>
    </validation-provider>

The flags dirty and pristine are not updated, whereas valid and invalid are.

See demo: https://codepen.io/dmoebius/pen/XvwLBb?editors=1010

Additional context
With vee-validate 2.2.3 is seems to work. You can verify this if you change the version in the Codepen demo. So the bug must have been introduced between 2.2.3 and 2.2.14.

@dmoebius
Copy link
Author

Seems to be caused by this code in addListeners:

  // add the validation listeners.
  this.normalizedEvents.forEach(evt => {
    addVNodeListener(node, evt, onValidate);
  });

this.normalizedEvents contains the input event, which means that addVNodeListener() replaces the previously attached input listener (which listened for dirty changes) with the new onValidate handler.

@dmoebius
Copy link
Author

Or the problem is inside mergeVNodeListeners() which always removes all previously attached vee-validate handlers (those with flag _vee_isUnique). Probably a follow-up of #2185?!?

@logaretm
Copy link
Owner

logaretm commented Aug 12, 2019

This is indeed introduced by #2185, 2.2.13 seems to work fine! I will revert it and push a quick update since this is a critical problem.

@logaretm logaretm added the 🐛 bug Unintended behavior label Aug 12, 2019
@logaretm
Copy link
Owner

2.2.15 is out with a fix.

@dmoebius
Copy link
Author

Ok, thanks. But your change reopens #2185, and the bug with the ever-growing input listener list...

@dmoebius
Copy link
Author

dmoebius commented Aug 12, 2019

This is inside the render function:

      extractVNodes(nodes).forEach(function (input) {
        addListeners.call(this$1, input);
      });

Is it really necessary to add the listeners in each render cycle? I mean, render is really called a lot! Furthermore, extractVNodes is a very expensive operation.

@logaretm
Copy link
Owner

Each time the component re-renders, the render function gets a fresh VNode, so we need to add them in each render cycle, this is exactly the same thing as @click in your Vue templates. in Either case Vue diffs the trees and doesn't necessarily remove/adds new listeners if the VNodes stay the same.

The "ever-growing" input listener list does not really occur in normal circumstances, I would need to figure out why the list is growing in the first place. As each render cycle should have new empty vnodes, so it shouldn't happen.

extractVnodes while recursive, should not be an issue because the HTML required to place an input inside a ValidationProvider is really tiny and wouldn't have a perf penalty. Still, I'm open to suggestions to improve it if possible.

Normally the time required to render a component with a validation provider is almost the time required to render its children so certainly no problems there unless you can point out cases where it would be horrible. (I tested the providers in a 120+ input and no penalty was observed #1191).

@dmoebius
Copy link
Author

Ok, thanks for the explanation. I saw the growing handler list once in the debugger, but cannot reproduce it now with the new version 2.2.15. You're right that Vue always provides new empty vnodes to the render function; I cannot find a situation where Vue re-uses vnode instances.

@logaretm
Copy link
Owner

That's the problem. It shouldn't re-use them. My only guess is that maybe the rendering is slow enough to allow the render calls to overlap.

AlvaroBrey pushed a commit to imagames/vee-validate that referenced this issue Jan 7, 2020
…garetm/vee-validate

* upstream/v2: (689 commits)
  fix: update v3 links and repo links closes logaretm#2467
  chore: add v3 docs link
  docs: add note for read me
  Added missing changes flag in FieldFlags definition (logaretm#2218)
  docs: uneeded document about escaping regex
  docs: fix the regex example
  chore: bump to 2.2.15
  revert: logaretm#2185 closes logaretm#2209
  chore: bump to 2.2.14
  chore: upgrade dependencies
  fix: mark vee-validate handlers so they get deduped
  Revert "fix: don't sync the value in an event handler closes logaretm#2185"
  chore: rm .DS_Store
  test: fix failing tests due to punctation
  test: fix failing tests
  Remove periods at end of validation messages (logaretm#2195)
  alpha_helper.js: regex for persian characters has been added (logaretm#2190)
  fix: don't sync the value in an event handler closes logaretm#2185
  fix: cleanup events before adding new ones
  Update displaying-errors.md (logaretm#2186)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Unintended behavior
Projects
None yet
Development

No branches or pull requests

2 participants