logaretm / vee-validate

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

Prevent creation of $validator on every component. #468

Closed lehni closed 7 years ago

lehni commented 7 years ago

Versions:

Description:

In the beforeCreate() hook of the vee-validate mixin, a $validator object is created for every component in Vue.

I think this is a bit excessive, and would like to propose that instead it is created only when needed, through a computed property that creates it on the fly on the first use. That way, only the components that use validation need to create this property. In my app, that's maybe about 5% of the used components.

mixin.js:

import Validator from './validator';

export default (Vue, options) => ({
  computed: {
    $validator() {
      const validator = new Validator(null, { init: false });
      Vue.util.defineReactive(validator, 'errorBag', validator.errorBag);
      Vue.util.defineReactive(validator, 'fieldBag', validator.fieldBag);
      validator.init();
      return validator;
    },
    [options.errorBagName]: {
      get() {
        return this.$validator.errorBag;
      }
    },
    [options.fieldsBagName]: {
      get() {
        return this.$validator.fieldBag;
      }
    }
  }
});

I haven't tested this yet but am fairly certain that it should work. I am happy to do the testing and create a PR if it's welcome.

lehni commented 7 years ago

What remains to be seen is if the call of validator.init(); in mounted() is required. If it is needed, but should only be called if a validator was created, then we'd have to use an internal property, e.g. _validator which gets created through the $validator computed property, and only if _validator is already set, mounted() would call init() on it. It can't use $validator anymore as that would again create a validator for all mounted components...

lehni commented 7 years ago

I am just realising that this change would allow for another welcome scenario: The possibility to share one validator across multiple child components. I have now tested this, and it works well in my tests:

  1. Remove the default beforeCreated() and mounted() hooks. It would be nice if this hack wasn't needed:

    import VeeValidate from 'vee-validate'
    Vue.use(VeeValidate)
    // Remove the VeeValidate beforeCreate() and mounted() hooks again that create
    // and assume $validator objects for every component:
    Vue.options.beforeCreate.pop()
    Vue.options.mounted.pop()
  2. Define a $validator property on the parent component that creates the validator. It is worth considering if this functionality that is already present internally in vee-validator could be exposed for such a scenario:

    computed: {
      $validator() {
        const validator = new Validator(null, { init: false })
        Vue.util.defineReactive(validator, 'errorBag', validator.errorBag)
        Vue.util.defineReactive(validator, 'fieldBag', validator.fieldBag)
        return validator
      }
    }
  3. Define a $validator property on the child components that retrieves the $validator from the parent. Note that this even allows for multiple levels of nested children:

    computed: {
      $validator() {
        return this.$parent.$validator
      }
    }
  4. Components that are going to use validation need to explicitly call $validator.init() from their mounted() callback:

    mounted() {
      this.$validator.init()
    }

And that's it! It just works.

Note that this would allow for a much simpler pattern to address the situation outlined in https://gist.github.com/sproogen/147d75db261505e8a558a7fd11a20551

logaretm commented 7 years ago

Thanks so much for the detailed analysis, I agree it is a bit excessive, but there are some concerns, the computed property acts as getters, so does the $validator computed property evaluate more than once? because we only need it to evaluate just once.

Also backwards compatibility, making the injection on demand is a big breaking change.

I looked a little bit into the issue in general and I would like to make use of the inject API provided by the Vue 2.2 https://vuejs.org/v2/api/#provide-inject

this way we can either, give the component the ability to either request a new validator instance or request the one that belongs to its parent probably by using different injection names. I think this achieves what you are looking for without much hacking.

What do you think?

Maybe at version 2.1 we can force this behavior.

lehni commented 7 years ago

Computed properties cache their values and are only evaluated again if something changes that their definition depends on, in a reactive manner. Since there is no outside dependency in the definition of the parent $validator above, this wouldn't be the case and they would only be created once. The children also cache their values for $validator, so access will be fast. There, they would reevaluate the cached values automatically once the paren't s $validator would change. But this doesn't actually happen, for the same reasons just stated.

As for inject / provide, I wasn't aware of its existence. It sure sounds like this would do exactly what I need: Shared $validator / errors objects across all components in a given hierarchy, with access to the errors on all levels.

The hack outlined above works for me already, and I will use it for now, but I would prefer to be able to switch to something officially supported in 2.1

logaretm commented 7 years ago

Right, because I was wondering if array mutations on the errors object will trigger revaluation or not. anyways I will work on it ATM, I could add a global inject option which is true by default, you can set it to false and use the inject and provide to get your instances. so you wouldn't have to wait for 2.1

in 2.1 we can force this behavior, and switch the status of the inject option to be false by default.

lehni commented 7 years ago

@logaretm that sounds great! Happy to beta-test it.

lehni commented 7 years ago

I just tested it and can confirm that mutations to the errors array by causing validation errors do not trigger a new execution of the $validator getter. So this works as it should.

johnhargrove commented 7 years ago

@lehni Thanks for this. Surreal to run into a very specific problem like this and find that someone else solved it 17 hours earlier. Your timing is impeccable. 😄

johnhargrove commented 7 years ago

I had to implement this a bit differently, partly due to the involvement of TypeScript, and partly because I used provide/inject. When using provide/inject you have to take into account the order of lifecycle hooks relative to the provide/inject operations. Basically, Vue performs provide and inject between beforeCreate and create. In my solution I did this:

  1. Initialize the VeeValidate plugin as @lehni did above. Hook removal hack, etc.
  2. In the parent, provide the validator:

    @Provide() $validator : Validator;
  3. In the parent, during beforeCreate create the validator. This makes sure the validator is created before the inject/provide happens. If you do it later, you'll inject undefined:
    beforeCreate() {
        this.$validator = new Validator(null, { init: false }, {});
        (Vue as any).util.defineReactive(this.$validator, 'errors', this.$validator.errorBag);
        (Vue as any).util.defineReactive(this.$validator, 'fields', (this.$validator as any).fieldBag);
      }
  4. In the children, inject the validator:
    @Inject() $validator : Validator;
  5. In the children, during create (this is called /after/ the injection happens, so the validator should exist) initialize the validator:
    created() {
        this.$validator.init();
    }

    Note that there is some weird casting in the TypeScript code. The constructor is also incorrect. The type definitions for this project are slightly out of date. I submitted a tiny pull request that would make this code a tiny bit cleaner: https://github.com/logaretm/vee-validate/pull/470

lehni commented 7 years ago

@johnhargrove thanks for sharing this! As I'm not familiar with TypeScript, could you explain what these lines do in pure JS?

@Provide() $validator : Validator;
@Inject() $validator : Validator;

I'm not clear on what actually happens there...

johnhargrove commented 7 years ago

Those are 'decorators' that are a part of the TypeScript compilation process for Vue. They basically mean that those variables will be placed in the inject: { } section of the generated JS viewModel.

A great side-by-side of them is here on the project page for those decorators: https://github.com/kaorun343/vue-property-decorator

See under usage.

lehni commented 7 years ago

Great, thanks! Here my JS version:

Parent:

import Vue from 'vue'
import VeeValidate from 'vee-validate'
Vue.use(VeeValidate)
// Remove the VeeValidate beforeCreate() and mounted() hooks again that create
// and assume $validator objects for every component:
Vue.options.beforeCreate.pop()
Vue.options.mounted.pop()

export default {
  provide() {
    const validator = this.$validator = new VeeValidate.Validator(null, { init: false })
    Vue.util.defineReactive(validator, 'errorBag', validator.errorBag)
    Vue.util.defineReactive(validator, 'fieldBag', validator.fieldBag)
    return {
      $validator: validator
    }
  }
}

Child:

export default {
  inject: ['$validator'],

  mounted() {
    this.$validator.init()
  }
}

@logaretm perhaps something like the parent's behaviour above could be exposed as a mixin on the VeeValidate object? e.g. VeeValidate.ValidatorMixin ? Just a thought...

lehni commented 7 years ago

@logaretm I just tried this out with your commit:

Vue.use(VeeValidate, {
  inject: false
})

And then I explicitly inject the $validator in my child components:

export default {
  inject: ['$validator']
}

This doesn't work though, since every component still receives the provide() method that creates their own $validator. I can't get the child components to share the ones from their parent. Am I missing something?

I think only the components that should provide a validator would need to define this provide() method. But I don't see a way to achieve this with your change.

lehni commented 7 years ago

Debugging this a bit further, it seems that even the components that provide a $validator need inject for it to be present / defined on themselves. Your change in 1ad51de9a80f517b831542b03fc977ba256285ce right now allows the selective definition of inject: ['$validator'] per component, but it doesn't allow for the prevention of the code in provide() to be executed, so each component that defines inject: ['$validator'] receives its own $validator, and the validators are never shared.

All that is needed now is a way to prevent the current version of provide() on selected components, or even better, to explicitly define it only on certain components.

One way to do so could be through a mixin (e.g. VeeValidate.ValidatorMixin). Another through an added option, e.g. this.$options.provideValidator: true. But that feels a bit hackish? I don't know what's best, but what's currently there still doesn't allow for the sharing of validators, as far as I can tell.

lehni commented 7 years ago

One last update, with the code and changes that make it work for me:

vee-validate makeMixin(), hacked to add provideValidator option:

  mixin.provide = function provide() {
    if (options.inject || this.$options.provideValidator) {
      var v = new Validator(null, { init: false });
      Vue.util.defineReactive(v, 'errorBag', v.errorBag);
      Vue.util.defineReactive(v, 'fieldBag', v.fieldBag);

      return {
        $validator: v
      };
    }
  };

Parent:

import Vue from 'vue'
import VeeValidate from 'vee-validate'
Vue.use(VeeValidate, {
  inject: false
})

export default {
  provideValidator: true,
  // parent needs to inject its own $validator for it to be present:
  inject: ['$validator']
}

Child:

export default {
  // children inherit from the parent:
  inject: ['$validator']
}

Final note: This looks to me that what's really needed isn't a control of the inject behavior. In either scenario, it is fine to always inject the $validator, it is then the existence the $validator in provide() that decides if the parent's $validator is used, or if a new one is created. So what we really need is a way to control that behavior on a per component level.

logaretm commented 7 years ago

I originally meant for the child components to request the validator instances in this manner:

So in a child component you are forced to use the object inject form:

export default {
  // children inherit from the parent:
  inject: {
    $validator: '$parentValidator'
  }
}

Which I think should work, though I haven't handled if the parent component can provide it or not, for example, if a parent component does not inject a validator instance, the child should receives null, I might create a NullValidator class to display warnings for that case, or provide a sensible behavior.

While the Boolean suggestion would work, but I think it is a bit hacky like you said. I haven't yet finalized how should child components receive the validator instance from their parents, just trying multiple approaches.

Edit: I fixed the parent validator injection using the above method in a recent commit.

lehni commented 7 years ago

I wanted to try your suggestion of using $parentValidator, but this gives an error in this.$options.inject.indexOf inside mixin.beforeCreate(), because inject is not an array now, but an object.

I then fixed that given line, but even then a simple scenario of a parent and a child component, with the child injecting the parent validator didn't work, causing other errors, related to errorBag...

I also logged the creation of the $validator object in mixin.provide(), and although I was using the $parentValidator in the child, the Validator constructor was still called in the child's provide() as well as in the parent, so this doesn't seeem to help with the prevention of the excessive creation of $validatorobjects.

I was also a bit surprised by the reliance on $parent in the defintion of $parentValidator: Isn't the whole point of the provide / inject pattern to not have to explicitely access parent components, and allow such property injection across hierarchies that are deeper than just one level down? With the reliance on $parent, this advantage would be lost.

I think what I need is this:

  1. A way to prevent provide() from creating $valdiator objects on every component.
  2. A way to tell some components explicitely to create the $validator object. As pointed out before, this could be done through a mixin that VeeValidate exposes.

And lastly, it looks to me that with this scenario, a inject: ['$validator'] on every component would actually be fine, and not the behavior that would need to be controlled through a config switch, as the property would then either be retrieved from the component itself or one of its parents, whoever provides it first.

lehni commented 7 years ago

One last observation:

I just realized that when a component defines a value in provide() and injects it in inject, it doesn not receive this value from itself; it receives the one from its parent.

So what I believe is happening with your code-base currently is this:

This should already have an impact on the behavior of vee-validate (probably undesired), where all direct children of a component share the $validator of their parent. It does not change however the amount of $validators created.

Instead, I think there should be a way to allow a component to:

The way I am achieveing this right now is:

[1]

export default {
  inject: ['$validator']
}

[2]

export default {
  beforeCreate() {
   // NOTE: Setting this.$validator is required for the shared validator
   // to be present on the parent itself.
    const validator = this.$validator = new VeeValidate.Validator(null,
        { init: false })
    Vue.util.defineReactive(validator, 'errorBag', validator.errorBag)
    Vue.util.defineReactive(validator, 'fieldBag', validator.fieldBag)
  },

  provide() {
    return {
      $validator: this.$validator
    }
  }
}
logaretm commented 7 years ago

Much appreciated, I don't think that does change the amount of validators created, I have pushed another modification to this, borrowing a lot of your suggestions:

There is a warning if the user attempted to use the directive with a component that does not inject the validator, which should be helpful when debugging issues related to it.

I initially thought adding a Boolean was kinda hacky, I guess it looks better with prettier name as like you said we need to tell the plugin if it should give the component a validator instance or not.

lehni commented 7 years ago

@logaretm thanks for the update. I hope to find time to test this later today or tomorrow!

logaretm commented 7 years ago

@lehni Take your time, I appreciate the time you already gave to provide feedback and helpful suggestions.

lehni commented 7 years ago

I just tested this now, and it works well for my use case!

Just one observation: I had a component where I forgot to provide the name attribute, and with this latest version, this now produces the following confusing error, while before it wasn't complaining. I think the error should be more explicit, stating that no field-name could be determined for the field:

[Vue warn]: Error in directive validate inserted hook: "[vee-validate]: Cannot add a listener for non-existent field ."
logaretm commented 7 years ago

Noted, I guess we can close this then.

Toilal commented 7 years ago

I tested this feature as I need it, and it works well.

It should be documented before release though, and TypeScript definition files should be updated to accept the new $validates component option.

Toilal commented 7 years ago

@see #491

Toilal commented 7 years ago

It seems there are some issues when children components are inside a v-for directive.

In this case, $validator is injected with undefined value from repeated children.

Toilal commented 7 years ago

Well, my issue is not caused by v-for directive, but by draggable. When draggable wraps children elements, $validator is undefined for them. (Maybe it's a draggable issue).

Toilal commented 7 years ago

I just created an issue with a JSFiddle reproducing this : https://github.com/logaretm/vee-validate/issues/492

abeltodev commented 7 years ago

Could you provide a final example? Thanks

logaretm commented 7 years ago

@abel888 It will be documented, and mentioned in the release notes.

I have changed a small thing, by default all components that has a validator, can provide it. meaning no need to turn inject to off to share validators between instances. setting it to false only disables automatic injection, so this is the priority list now:

This makes it simpler to use, and have no side effects to the previous suggested behavior.

amorriscode commented 7 years ago

This functionality is awesome and very useful. There is only one thing I would like to add.

It seems I cannot share scope between components. I would like to add an error to the scope of another component, but when I try it seems to remove the error. Because of this, I am checking for errors in two different scopes.

Here is an example:

This component has form fields being validated:

<div class="form-group" :class="{ 'has-danger': errors.has('email'), 'has-danger': errors.has('email', 'account') }">
    <label for="email" class="col-form-label">Email</label>
    <input v-validate="'required|email'" id="email" class="form-control form-control-lg" name="email" :value="user.email" @input="update($event, 'email')">
    <span v-show="errors.has('email')" class="form-control-feedback">{{ errors.first('email') }}</span>
    <span v-show="errors.has('account.email')" class="form-control-feedback">{{ errors.first('account.email') }}</span>
</div>

In another controller, I am submitting my form (it's a registration flow). If I get an error back that the email is already taken, I do this:

this.errors.add('email', error.response.data['user.email'][0], 'emailUsed', 'account');

If I do this, it works, but I have to check for two different errors on the form. I'd expect to be able to use the same scope for my form (first component) but when I do it that way, the errors get cleared from the bag.

No idea if there even is a workaround for this but I thought it would be valuable to mention the use case.

logaretm commented 7 years ago

Can you demonstrate it in a fiddle, I'm not sure if I follow. It should allow using scopes across components, since you are using the same validator.

chlab commented 7 years ago

Just wanted to say: the new injection functionality is awesome! Got rid of all my scope and event bus stuff, my code is much cleaner. Thanks for your great work @logaretm !

Mouvedia commented 7 years ago

Shouldn't calling validateAll on the parent $validator trigger the error messages on the sub components that have injected the parent's validator?

azampagl commented 7 years ago

@chlab Do you have an example/fiddle of how setting $validates shares the validator between parent and children. Did you still need to inject/provide for every component? I'm having the same issues as @Mouvedia where the parents validator does not recognize any of the rules attached in the children.

chlab commented 7 years ago

Validation works for me.

First of all, you must be using ^2.0.0-rc.4.

Disable automatic injection of new $validator instances in every component: Vue.use(VeeValidate, { inject: false })

On the children:

export default {
  inject: ['$validator'],
  // ...the rest of your component
}

on the parent:

export default {
  $_veeValidate: {
    validator: 'new'
  },
  // ...the rest of your component
}

validation: this.$validator.validateAll()

That's all it takes. Make sure you aren't using scopes anymore in case you were using them before.

Mouvedia commented 7 years ago

@chlab

  1. Are error messages of the sub component in the sub component? Are they triggered?

  2. is this.$validator.validateAll() called on the parent?

chlab commented 7 years ago

1 & 2. yes

Mouvedia commented 7 years ago

@chlab I am not able to reproduce it. As @azampagl said we will need an example. I am using rc.5

chlab commented 7 years ago

Set up a JSFiddle an I'll have a look

Mouvedia commented 7 years ago

I don't have time right now, sorry.

lehni commented 7 years ago

@chlab shouldn't scopes still be supported though? This sounds like a side-effect of the new approach...

azampagl commented 7 years ago

@chlab Got it working... I needed the $validates AND @Inject $validates: Validation (I'm using typescript).

akbansal commented 7 years ago

@chlab, @logaretm @lehni This is not working for me-- i have a complex form.. which contain multiple text field component and one another custom application component which again has some more input component. I tried your solution in advance Inject ..but when i check on the parent component it always has the error from parent component.. child component does not have any errors.. I wanted whenever i run $validator.validateAll(), it should run all validations including all child components and show the errors on the page.

chlab commented 7 years ago

@akbansal the feature is proven to work. Your comment does not really contain any information with which we could help you. If you want us to help you'll have to provide a jsfiddle demonstrating your usecase.

solidevolution commented 7 years ago

Same here. I work with a form in parent component and child components inserted there with router-view). Parent and child components have inputs to validate. The template validation works but the error bag works only for parent component.

Do you have an example how i can push the errors from the child component(s) to the parent component?

cristianqr commented 7 years ago

@akbansal, @solidevolution I also tried previous solutions but none of these does not work, so I had to solve it another way. Below is the code

bus.js

import Vue from 'vue'
const bus = new Vue()
export default bus

parent-component.vue

<template>
  <div>
    <form novalidate @submit.prevent="save">
      <h1>New Person</h1>
      <input type="text" v-model="person.firstName" name="firstName" placeholder="First name" v-validate="'required'" :class="{'invalid-input': errors.has('firstName')}">
      <cqr-child-component :person="person"></cqr-child-component>
      <button type="submit">Save</button>
    </form>
  </div>
</template>
<script>
  import ChildComponent from './child-component'
  import bus from '@/bus'
  export default {
    name: 'cqr-parent-component',
    data () {
      return {
        person: {}
      }
    },
    components: {
      'cqr-child-component': ChildComponent
    },
    methods: {
      save () {
        this.validate()
        console.log(this.$validator.errorBag.errors)
      },
      validate () {
        this.$validator.validateAll()
        bus.$emit('validate-all')
      }
    },
    created () {
      bus.$on('child-validated', (errors) => {
        errors.forEach(error => {
          this.$validator.errorBag.errors.push(error)
        })
      })
    }
  }
</script>
<style>
  .invalid-input{
    border: 1px solid #f00;
  }
</style>

child-component.vue

<template>
  <div>
    <input type="text" v-model="person.firstSurname" name="firstSurname" placeholder="First surname" v-validate="'required'" :class="{'invalid-input': errors.has('firstSurname')}">
  </div>
</template>
<script>
  import bus from '@/bus'
  export default {
    name: 'cqr-child-component',
    props: ['person'],
    created () {
      bus.$on('validate-all', () => {
        this.$validator.validateAll()
        bus.$emit('child-validated', this.$validator.errorBag.errors)
      })
    }
  }
</script>
<style>
  .invalid-input{
    border: 1px solid #f00;
  }
</style>

That's all it takes.

overcache commented 7 years ago

@chlab thanks , your code works.

But is there any doc about:

export default {
  $validates: true,
  // ...the rest of your component
}
chlab commented 7 years ago

@icymind there is, here

lehni commented 7 years ago

Please note that the setting has recently changed and is now:

$_veeValidate: {
  validator: 'new'
}

instead of

$validates: true