-
Notifications
You must be signed in to change notification settings - Fork 379
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
base: master
Are you sure you want to change the base?
Conversation
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.)
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).
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! |
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 |
I've always disliked the method name 'group' as well. What about creating a 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 |
+1 for having a |
I was thinking that the detail object in the array would be the extension point in the future. The current implementation of 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 |
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 Generally speaking, I think that having an object (with convenience methods like 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. |
I do like the sound of that - particularly the suggestions around async validations. Returning a promise from I'll try to find some time to look into this in a bit more detail and maybe throw an example together... |
@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. |
@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! |
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. |
if looking for a simple "[field label]: [validation message]" style error message approach, please see this stack-o |
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:
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:
I hope that this functionality can be incorporated into the core library.
Let me know what you think.
Best regards,
Pavel Volgarev