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

What do you think about adding another method to the model instance that actually does the validation on saving? #2

Closed
anyong opened this issue Mar 31, 2016 · 6 comments

Comments

@anyong
Copy link
Contributor

anyong commented Mar 31, 2016

Rough example:

module.exports = function (bookshelf, validator) {
  'use strict';

  let Model = bookshelf.Model.extend({
    validationErrors: function () {
        // existing code...
    },

    initialize: function () {
        // attach event listener to do the validations on save
        if (typeof this.validateOnSave === 'function') {
            this.on('saving', this.validateSave);
        }
    },

    validateOnSave: Promise.method(function () {
        var errors = this.validationErrors();
        if (errors == null) {
            return null;
        }
        // Throw the errors or something so the model won't save.
    })
  });

  bookshelf.Model = Model;
};

If you want to override initialize in a model then you need to do:

initialize: function () {
    bookshelf.Model.prototype.initialize.apply(this);
    // more initialization here
}

I'm currently using your plugin like this and it works really nicely, saves me a ton of boilerplate. Thanks!

@kripod
Copy link
Owner

kripod commented Mar 31, 2016

I decided to use a separate validationErrors method in order to get information about more errors at once. You could add an optional attribute which triggers validation automatically before saving the Model: I'd happily accept a pull request for that! 😊

Also, I think there should be a custom Error object made for throwing exceptions.

@anyong
Copy link
Contributor Author

anyong commented Mar 31, 2016

Also, I think there should be a custom Error object made for throwing exceptions.

This is possible, but then you would probably also want to have your validations look like this:

validations: {
    email: [
        {
            method: 'isEmail',
            options: {/* validator isEmail options object */},
            error: 'E_INVALID_EMAIL'
        }
    ]
}

and the corresponding error messages:

{
    email: ['E_INVALID_EMAIL']
}

instead of the current one which returns just the called method name.

That way you can throw a custom error message for each validation even if it uses the same validation method in different models.

With that setup though it would reduce boilerplate to almost nothing, and just adding the validations object to a model would do pretty much everything you need.

@anyong
Copy link
Contributor Author

anyong commented Mar 31, 2016

Also, not sure if you're interested but I refactored the plugin just to be a bit cleaner, get rid of the new Function eval, changed all of your let to const where possible, and got rid of the continue statements in favor of control flow logic.

If you want to use that, I can make a pull request with it and for the custom error and auto-validate on save stuff after I write that.

@kripod
Copy link
Owner

kripod commented Mar 31, 2016

That would be great, but please, submit the Error emission feature as a separate pull request.

I'd suggest to create a new ValidationError object with a data argument which shall return the values returned by the Model.validationErrors() function instead of adding an error property to Model.validations.[property]

@anyong
Copy link
Contributor Author

anyong commented Mar 31, 2016

Ok this is working really well, I'm happy with the result. I also recommend not adding the special isRequired checks in the Plugin, but rather onto the validator object as a normal method.

I'll make a couple of PRs in a minute...

@kripod
Copy link
Owner

kripod commented Mar 31, 2016

Thank you very much for all the help you provided! I highly appreciate all of your efforts towards my project.

@kripod kripod closed this as completed Mar 31, 2016
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

No branches or pull requests

2 participants