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

Extended validation details. #298

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

volpav
Copy link

@volpav volpav commented Jul 5, 2013

We're using Knockout Validation quite extensively on one of our current projects and one of the requirements for this project is that when client-side validation fails, the system should:

  • Highlight those fields that didn't pass validation.
  • Show validation summary, a section at the top of the page with the complete list of validation messages where every message has a format [field label]: [validation message].

Basically, what I needed is an ability to get from Knockout a bit more detailed information on every validation error (not just its textual representation which is available already). Turned out, it's not possible without creating custom rules or performing some hacks.

Here's how the new functionality works:

// Getting errors
var errors = o.validation.group(this, {
    deep: true,
    observable: false
});

// New method: getting extended details
var details = errors.getDetails();

for (var i = 0; i < details.length; i++) {
    var d = details[i];

    /*
        Every element contains the following fields:

        "observable" - a reference to the target observable.
        "error" - the error message.
        "rule" - the name of the failed validation rule.
        "data" - an object that contains extension data (provided via "extend" method) for every rule. E.g. "data.required == true".
    */
}

I hope that this functionality can be incorporated into the core library.

Let me know what you think.

Best regards,
Pavel Volgarev

Added an ability to retrieve extended validation details (e.g. the name
of the failed validation rule, parameters for all rules configured on a
target observable, a reference to the target observable, etc.)
@stevegreatrex
Copy link
Contributor

Hi @volpav,

I like the general idea of this functionality and I think it would form a good basis for however we choose to progress the group functionality in the next version. This is an area that will need some reworking in any case as the API isn't fantastic at the moment.

Personally I think that the implementation would be slightly neater if we took something like what you have written here as the 'base' functionality and then expose the existing functionality by mapping from those values. It seems cleaner to have one implementation that iterates the validatables and exposes the full detail, then to have a wrapper around that to expose a plain message list.

Thoughts?

If you could write some unit tests around the new functionality then we can look at refactoring both to come up with the best solution.

Tests for "getDetails" (extended validation results).
@volpav
Copy link
Author

volpav commented Jul 8, 2013

Hi @stevegreatrex,

Sorry, I'm not aware of the development plans and so I tried to make the change as "integrated" into the existing concepts as possible. Another reason why the change is implemented this way is that I was hoping that it could be quickly accepted and so we can just keep using the "official" builds (instead of our own fork). As for the "group" method, I definitely think that what it returns now must be changed. It should probably be an object representing a validation result. The object could look like the following:

return {
    errors: [Array], // Basically what "getResults" returns right now
    model: [object], // A reference to a model being validated, can be useful when passing result within the app
    isValid: function () { return !this.errors.length; }

    // Other things
};

Note that the "errors" is encapsulated as a field. A structure like this will make it possible to continuously enhance the functionality by adding additional fields and methods with less risk of introducing breaking changes.

Having an object will probably also make it easier to generate type definitions for languages like TypeScript (we currently use it within the project and existing definitions work really great).

I've added a couple of tests as you requested.

Cheers!

@delixfe
Copy link
Contributor

delixfe commented Jul 8, 2013

Hi @stevegreatrex, hi @volpav,

we really need functionality like that. I had the the same issues a year ago and have created the pull request #226 five month ago. It has another approach as you can configure ko.validation to generally return details.

Regards

Felix

@stevegreatrex
Copy link
Contributor

I've always disliked the method name 'group' as well.

What about creating a ko.validation.validate(target, options) method that returns an array of detail objects as described by @volpav above that we then wrap with a couple of computeds to duplicate both the current and the requested functionality?

var errorDetails = ko.computed(function() {
    ko.validation.validate(this)
});
var errors = ko.computed(function() {
    return ko.utils.arrayMap(ko.validation.validate(this), function(detail) {
        return detail.error;
    });
});

The validate method would be very similar to the existing group method in implementation

@volpav
Copy link
Author

volpav commented Jul 8, 2013

+1 for having a validate method in place of group but, @stevegreatrex, wouldn't you prefer returning an object (with the structure similar to what I proposed) instead of returning an array of details? Don't you think we'll end up monkey-patching it the same way we do now with "errors" (e.g. adding showAllMessages and getDetails to it)?

@stevegreatrex
Copy link
Contributor

I was thinking that the detail object in the array would be the extension point in the future.

The current implementation of showAllMessages, getDetails and errors could all be implemented by mapping the detail objects in the array, and (to my mind) it makes sense to return an array of results when you are validating an array of observables.

It's a little similar to @delixfe's implementation, where he has added the extra data to each observable (and not a summary of all observables).

I can see the use of the object for summary functions/properties (like the isValid in your earlier example) but I'm not entirely convinced that you would want to return them from every call. I wouldn't need a lot of convincing though!

@volpav
Copy link
Author

volpav commented Jul 9, 2013

These are my thoughts, hope it all makes sense to you.

I was always thinking about validation as something you apply to a model rather than to a set of properties (observables). Therefore, what you get back is an object that doesn't only expose the validation results but also carries some contextual information that can be passed further to some generic logic. After all, fields like model doesn't possess any overhead since it's all about passing references.

Generally speaking, I think that having an object (with convenience methods like isValid or showAllMessages) is much better approach in terms of maintainability and further feature development: new fields/methods can be easily encapsulated without changing the rest of an object shape which will most likely break some of the existing code. This approach also means that all the information related to a given invocation of a validation procedure is available all at once and so there's a central point in the API for obtaining these details This goes in contrast with having multiple methods providing information about the same, most recent invocation. Here I can see at least two major potential issues: an overhead of internally keeping (and maintaining) the state of the most recent validation procedure as well as the (unnecessarily complicated and confusing) API structure.

Another benefit of returning an object rather than an array is the ability to return promises in case of asynchronous validation. This can lead to a really powerful and efficient techniques when writing asynchronous applications:

var result = ko.validation.validate(this); // some of the rules are async
result.done(function(r) {
    // "r" and "result" reference the same memory (again, for scenarios when "result" is not available in the current context
});

These were some of the things I was cultivating in my mind during the day. Let me know, you what think.

@stevegreatrex
Copy link
Contributor

I do like the sound of that - particularly the suggestions around async validations. Returning a promise from validate makes a lot of sense.

I'll try to find some time to look into this in a bit more detail and maybe throw an example together...

@emes001
Copy link

emes001 commented Jan 13, 2015

@stevegreatrex @volpav is there any update on this issue (now close to 18 months old)? I have to replicate almost the exact same requirements as specified in the initial post.

@volpav
Copy link
Author

volpav commented Jan 14, 2015

@emes001 Not that I'm aware of, unfortunately. Feel free to cherry-pick what you need and reach out to me if you have any questions on how this extension works (although, I think it's pretty straightforward).

Cheers!

@mikocot
Copy link

mikocot commented Jan 11, 2016

It seems to me that the big part of it was introduced with the #449 , am I wrong? If so the topic can be closed.

@Beej126
Copy link

Beej126 commented Mar 29, 2016

if looking for a simple "[field label]: [validation message]" style error message approach, please see this stack-o

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

Successfully merging this pull request may close these issues.

None yet

6 participants