aurelia / validatejs

Enables expressive validation using decorators and/or a fluent API.
MIT License
22 stars 23 forks source link

Problem with Validatejs approach and UI's #45

Closed apawsey closed 8 years ago

apawsey commented 8 years ago

Unfortunately I think there's a bit of a core issue with the approach of this plugin. I don't say that to be alarmist or critical, but let me explain.

From what I can see, validatejs was designed to validate objects (taken from the first line of their site...

Validate.js provides a declarative way of validating javascript objects.

Now this is great in the sense of validating a viewmodel, say before posting to an api or some other action. However in a UI environment, the generally accepted UX is to progressively validate the input after the user has had an attempt - ie, we tell them where they make mistakes (everything's green, until they do it wrong, and then it goes red) instead of saying everything's a problem until they fix it (everything's red when they start and only goes green when they get it right)

The problem is our UI is tied to our viewmodel, which we are validating in it's entirety as an object. Therefore our forms start green, until the first time validation is triggered, which in a good form, is the moment the first field 'blurs'. Then our whole object is validated, and all feedback is presented, resulting in the rest of the form being marked negative.

As far as I can see, we would need to do something like exclude properties from validation, until that ui field has experienced an onblur event... and that already sounds pretty messy.

EisenbergEffect commented 8 years ago

@PWKad This is of critical importance. As we spoke the other day, fields that have not been touched shouldn't be validated. We need to make sure that works correctly.

plwalters commented 8 years ago

I'm not a big fan of mixing validation and tracking the state of objects. This should be really easy to do if you are using some sort of pristine tracking system which you can easily do in your view-model. I don't think mixing in state-tracking in to core validation is a good idea. If the problem is that you don't want to show UI errors until some event occurs perhaps that's an area that the templating side of things needs to be enhanced with but there are too many scenarios to cover everything by convention in my opinion. ex- should a property be validated if it hasn't been changed yet? What if someone manually calls validate while a form is being submitted?

apawsey commented 8 years ago

While I agree in principle @PWKad, I don't see how you're going to get around it with the way the validation is put together.

The question here is what are we validating... the UI or the object? If the purpose here is to validate the viewmodel, then we need to drop the support for directly rendering the errors, as we're agreeing the UI is not our concern. In which case then we'll have to start another project for Aurelia UI Validation, which allows to process the results of viewmodel validation to the UI.

However if aurelia-validation and it's implementations are supposed to be working with the UI, then I can't see another choice but to take the UI into account in more detail. I can see two solutions.

The first is the one you like the least, that the validate binding needs to know if the property is dirty or not (tracking the dirty state can be configured on the binding if possible, defaulting to true). If something's changed it, not necessarily the user (because maybe they clicked something that filled in the form for them), then we show errors, otherwise we don't. On top of this, we overload the validate method with an additional parameter that overrides dirty tracking (or whatever you want to call it).

The second solution is that the binding's do not trigger validation of the whole object, but rather only their property. However with validatejs, this would be a bit complicated. You would either need to call the validate function with the full object, but with only the constraints for the field you want to validate, or construct a new object with only the property you want, and then either a single constraint, or all of them. The downside of this solution is that if someone wants to use the reporter/observer event...

this.reporter = ValidationEngine.getValidationReporter(this);
        this.observer = this.reporter.subscribe(result => {
            this.validationErrors = result;
        });

They will receive varying levels of validation, and not the whole list of errors.

Although I understand it's not liked, in my opinion the first solution is the least intrusive option, where at least the complication is contained within the binding. You make it the binding's responsibility to track this complication, otherwise the rest of the validation continues to work as is. That also means that alternative implementations of bindings, renderers, etc, can all handle this as they see fit. All of this is obviously predicated on the ability for the rest of the the aurelia components to support this scenario.

I hope none of that sounds too forward.

EisenbergEffect commented 8 years ago

@PWKad This isn't negotiable. As @apawsey has rightly indicated, this is essential for building a validation system that works for UI. Without this, I would consider it fundamentally broken and probably unusable.

Think about how a user interacts with a form. If there are five fields on the form, and they press one key in the first field, they don't expect to see errors on all the fields. They would expect to see errors on only the first field that they are interacting with. We can control the frequency already. This is handled by binding behaviors. So, we already have the problem of keyup vs. blur solved. What we need to have in the UI is the ability to only validate what is being interacted with.

We don't want to validate the whole object when someone is just inputing one value. This and whole object validation are two separate scenarios we need to support.

stevies commented 8 years ago

Sorry to jump in here. Especially if the new validator already allows this. But I think it is important to also be able to switch off in-line validation, and only trigger it when - say - the form submit button is clicked. Then immediately turn it off again whilst still showing the original errors to prevent the page jumping around when the user starts correcting incorrect data in the form.
See:

http://govuk-elements.herokuapp.com/errors/example-form-validation-multiple-questions

for an example of how I see this working (OK, that is doing some server-side validation, but you get the idea). Not every use-case demands inline, automatic validation. That needs to be configurable.

My brief experiments appear to show that the fluid validator in the sample app does it like that. But the non-fluid one does not.

apawsey commented 8 years ago

That's why I suggested the binding be configurable (this is the point I'm not sure on, don't even know if you can have configurable bindings - maybe multiple bindings?). If I use the 'dirty checking' binding, in-line automatic validation occurs, if I use the other binding, it only shows errors when it receives them, ie: when the validate method is called. The validating of the whole object is fine in my opinion, it's just about how we present it to the user. Like I said, the reporter can always report on all the errors, the issue is just the ui elements only displaying them when configured (or instructed) to do so.

stevies commented 8 years ago

Agreed. Just thought I'd add my own specific use-case in here to add to the argument for full configuration.

EisenbergEffect commented 8 years ago

@apawsey @stevies Thanks for the feedback. I understand and agree. We could potentially have the validate binding behavior handle these different visualization timings and let that be configured either through passing it a parameter or adding some custom attribute or config via the DI.

The binding behavior has access to the view's DI, so what if your view model could optionally have a "ValidationController" injected to let you control the behavior? The ValidateBindingBehavior could gain access to this through the View's container. Because DI is hierarchical, you could configure the behavior at the app level, and then also override it on a per View basis.

So the pieces would break down as follows:

ValidationEngine and ValidationRenderer would have varying implementations depending on the validation and css/widget libraries respectively. The other implementations should form a standard mechanism for validation in Aurelia.

Does this seem reasonable?

EisenbergEffect commented 8 years ago

As a side note, with what I've laid out above, you can see a bit of an MVC pattern emerging where the validation is the model, the validation renderer is the view and then there's the validation controller. We also have something akin to a binding system if you consider how the reporter works. Just a thought that occurred to me after I put this all down.

stevies commented 8 years ago

I think that makes sense.

plwalters commented 8 years ago

@EisenbergEffect re: https://github.com/aurelia/validatejs/issues/45#issuecomment-218447476 - we've talked repeatedly about moving the renderer and other templating logic into a separate library and putting more of the tie in to the UI in there as opposed to core validation. You even created this issue that notes we need to move it out. I'm not sure why you mention This isn't negotiable when we just talked about it and that was the decision (to move UI pieces out in to the separate repo and leave core validation to be just that - not templating / UI logic)

If someone wants to put together a branch showing the proposed changes in this discussion in place and it's reviewable that would probably be best.

plwalters commented 8 years ago

As a note it is currently supported to only validate single properties of objects and have the reporter report that out. This is to support the ability to have the bindingBehavior, renderer, or anything else only see errors that are relevant to what it cares about.

https://github.com/aurelia/validatejs/blob/master/src/validator.js#L13

this.validator.validate('firstName');

So it's completely possible (and easy) to create a bindingBehavior that waits to be triggered and show validation without baking that in to core validation.

export class MyBindingBehavior {
  bind(binding, source) {
    source._dirty = false;
    binding.target.onblur = this.blur;
    reporter = this.getReporter(source);
    reporter.subscribe(errors => {
      let relevantErrors = errors.filter(error => {
        return error.propertyName === targetProperty && source._dirty;
      });
      this.renderer.renderErrors(binding.target, relevantErrors);
    });
  }
  blur() {
    source._dirty = true;
  }
}
EisenbergEffect commented 8 years ago

@PWKad The thing that isn't negotiable is not having a validation library that can handle displaying the errors on a per field basis. What I'm hearing from the community is that our design doesn't make that possible. I'm not sure if that's the role of a renderer or not. If it is, our default renderer needs to work that way. Does that make sense?

EisenbergEffect commented 8 years ago

My idea was to extract a ValidationController so that it could be used by the binding behavior to help the renderer get this right. I don't want everyone having to create custom binding behaviors. In my opinion the end user should only need to create validation library adapters and renderers, but the rest of the pieces should just work.

EisenbergEffect commented 8 years ago

We may be mis-communicating or have a different idea of what the Renderer should do.

EisenbergEffect commented 8 years ago

My main concern is that we can support having a set of validation rules, binding a model for validation and showing the errors only as each field is edited. The thing we don't want is to have three fields, and as soon as you type something in the first field, you get errors on all three.

jsobell commented 8 years ago

OK, so just to throw a small kitten in with the pigeons; Are we sure we're approaching validation in the right way overall? Model validation is awesome, as it ensures the rules follow an entity around, avoiding duplication and inconsistency. But, issues I see include:

  1. without an ORM the task of tracking changes, pristine states, etc. gets messy
  2. model based validation is relatively unimportant given that full server-side validation of entities is still required because no entities can be trusted
  3. javascript is frequently used returning pojo objects in json structures, which are not objects and have to be (sometimes extensively) re-hydrated to allow validation
  4. many views only display a subset of fields, so what happens when non-displayed fields fail? Are we performing run-time consistency checking or input validation?
  5. links between properties and visual; elements become complex (as seen by the number of comments here) and users will struggle if it's not simple

The problem with much of what seems to be being discussed is that it's a little too ivory-tower, and too little conventional. Most developers would be happy with an attribute on an input field specifying the type of validation to perform, and a consistent means of displaying those errors, through classes being added to fields and optionally lists of validation failings. Sure, we can have model based stuff at some point, but do we need this complexity at this stage in the release?

The code I wrote for this used the aurelia-validatejs (because I like validate.js - it ticks all the boxes), and I created a custom validate= attribute that accepts the rule, and that attribute's VM walks up the DOM tree to find an encapsulating <validateblock> tag. This tag represents a unit of validation, and stores its failed validations via a DI injected ValidationService (this I'm not wild one, but I can't see a simple alternative). My intention in this was to allow multiple groups of validation per form, access to named 'validationblocks', simple conventional change event triggers to avoid the 'pristine' tracking, and a means of locating every validation attribute VM when needed.

There are wrinkles, and not being a javascript guru I wouldn't publish my version, but the point is that there are patterns that are simple and easy for people to understand, and easily achievable in a very short time. Let's think of it as aurelia-uivalidation. I don't care if the solution uses this type of approach or is a simple wrapper for validate.js, but I'm concerned that we're going to get bogged down in details over this, and I don't think there can be a credible RC without some decent, easily understood UI validation.

FYI, just as a thought-invoker, my experiment app has this sort of syntax:

         <input type="text" value.bind="firstName" class="form-control"
               id="fn"
               placeholder="first name"
               ref="firstnameInput"
               validate="rule.bind:{presence: true, email: true}">

etc. Not ideal, but simple and effective, and extensible through the VM bindings

ashleyschuett commented 8 years ago

If a field is required with no other validation the UI no longer shows an error if you leave a required field with no text in it. It also doesn't show the error in the subscriber callback of the reporter.

Is there something extra that needs to be done after the updated? If a field was required it would show an error in v2.1.

plwalters commented 8 years ago

@jsobell would this be a more simple way to accomplish the same -

<input value.bind="firstName & required" />

or with config for validatejs -

<input value.bind="firstName & required: { message: 'rofl. need this ^^}" />
plwalters commented 8 years ago

@jsobell @stevies @apawsey Can you please review my last comment and the referenced commit to see if this is an approach that's amicable here? If so I can add the other binding behaviors similar to the decorators and refactor the methods in to a helper module.

jsobell commented 8 years ago

Yes, I don't see any issue with this solution so long as it doesn't clash with any other binding behaviors. My working solution was intended to retain control of the validation process in the <validationblock> element, which instantiated a transient DI injected validatorService instance. This ensured a singleton per block of required validation, and allows multiple different 'areas' of validation on complex forms. The reference to the validationService is available via a 'ref' style attribute on the validationblock element, providing the VM with a means of accessing the current instance for a validate() method and access to lists of errors, but on a per-validation group basis. I'd love to see something which covered both model and UI scenarios, so fleshing out your suggestion is my preferred option, although I would also like to see a solution that doesn't require 'new'ing up a validation object in the VM. So a few questions: How would this method handle nested components such as composed controls? Is it feasible to provide multiple validation blocks, or must every visible element on a VM belong to the same validation group? Would this UI based system hook into change events to avoid the 'validate everything' type of issue, or will it trigger its validation tasks from the bindings engine? How would a user bind to validation errors in a way that that is extensible visually? I.e. for a field with <input value.bind='firstName & required & isemail' id='fn' Would there be a means of saying <div if.bind='isvalid("fn")'>This is just not kosher!</div>? This is something that I currently can't do in my solution because we still don't have a means of either forcing dirty-checking or computedFrom on a function!!!!!

EisenbergEffect commented 8 years ago

@jsobell For function re-evaluation, you can use signals or a silent input property to the function which you toggle to trigger re-evaluation. Both techniques would be more efficient than dirty checking.

jsobell commented 8 years ago

@EisenbergEffect With all due respect, I'm not in agreement with the suggestion that we encourage those workarounds in situations such as these. There are two issues to consider:

  1. Using signals involves adding code to the UI bindings that the UI developer has to know exists, effectively meaning the UI needs to have knowledge of 'magic' events. It's also messy and intrusive, particularly if you have a method that needs to be called if any property is updated in a model, meaning every property setter requires a trigger call, and every UI dependent tag requires a trigger detector. This doesn't look great on my permissions screen with 36 bit combinations.
  2. Adding a silent property (or perhaps several) exposes your implementation to the UI, and someone adding a call to if.bind='hasPermission(userid, "Delete")' has to know that they actually need if.bind='hasPermission(userid, "Delete", permbits, maskbits)'

We know that dirty checking is less efficient than either of those, but as a developer I also know when I want dirty checking and when I don't. If we really want to protect people from their own stupidity making naive mistakes, then we shouldn't have an event aggregator :) We already have dirty checking for getters, because there is no easy way to detect those changes either. At some point we would hope that aurelia/compute features will replace both of these, but for now I feel (and experience) the need for a working means of providing this feature.

I definitely don't want a movement to transition to dirty checking from the existing property intercept change detection, but I would like to see the option to use a framework based dirty check if I need it and my design justifies it. It can be another binding property, such as & dirtycheck or & dirtycheck(100) where the check period is configurable, perhaps also with an @observable() parameter as an alternative. It should definitely be opt-in, not opt-out, as I hate dirty binding too, but there are times where 100 lines of workaround and code obscurity make it the best solution.

jasoncarreira commented 8 years ago

I've gotta agree with @jsobell here. Aurelia is a great framework for the UI part of an app, and I love having a full domain model, but this is mixing concerns. We need a simple validation framework for the UI layer which mostly means form inputs. I may or may not put a fuller set of validations on my domain model, but I don't need Aurelia to provide this for me and the whole MVC of validation is just getting more and more complicated to cover handling too many cases.

If you want to build a nicer API for validate.js for doing domain model validation that's great... but contribute it to validate.js and keep the aurelia validation simple and UI-focused.

EisenbergEffect commented 8 years ago

@jsobell I didn't say say what I recommended was ideal. It does solve your problem as a workaround until we have something better.

y2k4life commented 8 years ago

@PWKad I'm at the point of adding validation to my app. I prefer, what I call, validate as you go method, instead validate everything upon a submit (UI vs View-Model?). I tried to implement what I believe is the commit that has ui-specific binding behavior. I'm getting an error. I confirmed that I have the correct source (in theory) by looking in the ./src/binding-behaviors folder for required.js.

The error is 'gulp-match: vinyl file required'

>git clone --branch custom-renderer https://github.com/aurelia/validatejs.git custom-renderer
Cloning into 'custom-renderer'...
remote: Counting objects: 775, done.
remote: Compressing objects: 100% (28/28), done.
rReceiving objects:  96% (74emote: Total 775 (delta 8), reused 0 (delta 0), pack-reused 745
Receiving objects: 100% (775/775), 152.61 KiB | 0 bytes/s, done.
Resolving deltas: 100% (491/491), done.
Checking connectivity... done.

>cd custom-renderer

>npm install
.
.
.
Bunch of NPM stuff, but no errors just warnnings.
.
.
.
>gulp build
[15:07:38] Using gulpfile C:\Projects\aurelia-validatejs\custom-renderer\gulpfile.js
[15:07:38] Starting 'build'...
[15:07:38] Starting 'clean'...
[15:07:38] Finished 'clean' after 69 ms
[15:07:38] Starting 'build-index'...
C:\Projects\aurelia-validatejs\custom-renderer\node_modules\gulp-match\index.js:7
                throw new Error('gulp-match: vinyl file required');
                ^
y2k4life commented 8 years ago

I also want to add that I did some sanity checking by building the main branch.

gulp build [15:22:07] Using gulpfile C:\Projects\aurelia-validatejs\validatejs\gulpfile.js [15:22:07] Starting 'build'... [15:22:07] Starting 'clean'... [15:22:07] Finished 'clean' after 52 ms [15:22:07] Starting 'build-index'... [15:22:07] Finished 'build-index' after 51 ms [15:22:07] Starting 'build-es2015-temp'... [15:22:07] Starting 'build-es2015'... [15:22:07] Starting 'build-commonjs'... [15:22:07] Starting 'build-amd'... [15:22:07] Starting 'build-system'... [15:22:08] Finished 'build-es2015-temp' after 1.3 s [15:22:10] Finished 'build-system' after 2.94 s [15:22:10] Finished 'build-amd' after 2.95 s [15:22:10] Finished 'build-es2015' after 2.95 s [15:22:10] Finished 'build-commonjs' after 2.95 s [15:22:10] Starting 'build-dts'... [15:22:10] Finished 'build-dts' after 15 ms [15:22:10] Starting 'clean-temp'... [15:22:10] Finished 'clean-temp' after 5.65 ms [15:22:10] Finished 'build' after 3.09 s

y2k4life commented 8 years ago

Found the error. Line 1 import {ValidationRenderer} from '../validation-renderer'; has ../ changed to ./ Line 3 import {ValidationEngine} from '../validation-engine'; has ../ changed to './'

y2k4life commented 8 years ago

I might have misunderstood the purpose of this discussion, but after hacking through the code a bit I think I have a bit more of an understanding, but at the same time a bit confused. Let me state my assumptions first.

  1. There are rules to define what to validate and how to validate it. Validate email address, make sure it is filled in and make sure it passes a regular expression test.
  2. There is a time when to do validation. This can be done upon an event, for example submitting a form and the whole view-model is validated. There is the option to validate upon blur or what I call validate as you go.
  3. There is the rendering of errors after the validation action takes place.

Correct me if I'm wrong but this is a discussion is about item 1 where to define the rules, either in the UI (view) or the view-model and not so much 2, or 3? Or is it all three?

Currently the rules are defined in the view-model. Then @PWKad made some changes for the required rule to be a binding behavior allowing the rule to be defined in the view. I got that to work, it required a little bit more modification to the code than what I stated previously. I had to make changes to the file structure, I had to move require.js out of a sub-folder and into the parent folder. I also renamed the file to follow a convention I see of *-binding-behavior, I ended up with require-binding-behavior.js. I also had to fix missing imports, add a configure parameter, changes to index.js to make it a global resource, etc...

I think my issue now is that in my test app, similar to the samples in the code base, I have a button that triggers the validation on the whole view-model. How do I tie the validate function in the view-model with the rules defined in the UI?

    validate() {
        this.validator.validate();
    }

I'm also leaning towards a solution that allows me to validate upon blur item 2. @PWKad you mentioned something above about this. It would be nice that once the user leaves a field for example a Zip Code field that I can validate just the zip code at that moment in time. In the required biding behavior code I did not see any logic calling the validate function of the Validator object. I would also assume that blur validation is not the only validation that would be done. Validation as you go without a safety measure of validating the whole view-model when submitting the data can lead to validation rules that fail. For example nothing is filled in, and no blur events are triggered to cause validation, and if there is no final validation upon submit then required fields will be blank, etc...

I will continue to hack.

plwalters commented 8 years ago

@y2k4life thanks for the feedback. I'm updating that branch now and doing some monkeying around to get things working like they are supposed to be. Currently there isn't a great way to do what you mentioned above. The problem is that everyone has a different way they want to use or define things so it's a bit difficult to come to conclusions.

jsobell commented 8 years ago

Yes, this covers part of it, although there are other considerations such as what it is that is actually validated. The current implementation validates the data in the VM, while another approach is to validate the contents of an input element, so in one case we have an external validator monitoring a series of properties, while in the other we have an external validator monitoring the content of an HTML input element. Yet we can also add the validation rules against the VM property, against the DOM Input element, or as a loosely coupled pointer to one of those by some form of selector. Also, when a validation rule fails, is it the property that's invalid, or simply the rule itself? If we say that the sum of two fields should add up to 100%, what's invalid if they don't? It's not really the fields, as each are valid in isolation. Soooo many questions :)

@PWKad Your suggestions https://github.com/aurelia/validatejs/issues/45#issuecomment-220795859 help to move the validation into the UI layer, although having the ability to validate all of the rules (for example on submit) looks tricky without some scope or group to which those validations belong. Most validation systems link themselves to a surrounding <form> tag, which I'm not wild on, but at least it means that an 'all inputs' validation check knows the rules that must be checked. Also, if we have a rule such as numLeft and numRight must total 100%, this ideally requires a VM function. If we now have a rule saying numTotals==100 how would that be bound to the UI? A div saying <div value.bind='numTotals & required'></div>? I.e. how would we refer to the rule definition? I think linking all of the validatable inputs to the underlying validation object is the only practical way around that, so we have <div if.bind='!validator.isValid("numRule")'></div> and numRule is a validation defined directly on the validator.

Rather than writing dozens of lines of what-ifs and ideas, how about if we have a list of implementation examples, so we can ask "How would this support..." style questions to prove it?

y2k4life commented 8 years ago

@PWKad I get the complexity I was just thinking maybe an option on the *-binding-behavior to indicate validation when 'blur' happens. The validation on the whole view-model is a given upon submit.

@jsobell I believe I understand what you are saying, validate the data in the HTML before it goes to the VM and if it fails then report the error, but don't store it in the VM. If I try to extrapolate the challenge, would this require hooks to call validation as the binding of the data from the HTML DOM to the VM is happening? If those hooks are not in the core then this would be a challenge. That is why I assume validation is happening on the VM and not the HTML data.

Validation is complex, in most cases it is simple, check if the field is filled in. In other cases such as the two inputs equal 100%, it is complex. I have a form that I ask a question with radio input, with options A, B, and C. Based on how it is answered more questions are asked. There are question for option A, that are different than option B, or C. The additional questions for Option A are required if the answer from the first question is A, but the questions for option B and C are not required.

In my situation I don't envision any validation framework to solve my problem. But, what I expect are ways for me to use the existing components (validation-reporter, validation-rendered) that are used for simple validation for my more complex validation. I will use a mix of custom validations and out-of-the-box validation like required, regular expressions, number, etc... If my complex fails I just add the error to the rest of them and let the framework handle displaying them.

I'm coming from ASP.Net MVC framework, I would add validation rules to attributes then check if the the model was valid. I could also do my own complex validation and add errors messages via the ModelState.AddModelErrors method. I'm not saying it is correct or not and I know it is a completely different platform than this. But one the thing in that paltform that is the same, validation of the model not the HTML. I'm just stating an example of where I could mix complex with simple validation but use the same framework to display errors

Before I get to my more complex validation I'm just trying to get some of the basic ones working, like required. @PWKad I will just stress though the inline (I think I have heard it as that) or the validate as you go option as being highly desirable. It would be nice from a UX perspective. From user feedback it is discouraging to fill out a form only to get to the end to find out phone number was wrong instead of finding out after you entered it. Then trying to scroll back up a form to find the invalid inputs.

y2k4life commented 8 years ago

@PWKad I almost have my desired effect, but the target or targetProperty is not correct. I have a value.bind="model.currentLocation.addressLineOne & required" in a VM called EnterLocatoin. I'm always getting a validation error, even with data filled in. What I get for target is EnterLocation and for targetProperty I get addressLineOne. I'm guessing what I was expecting to get was 'EnterLocation' and 'model.currentLocation.addressLineOne'. That or EnterLocation.model.currentLocation and addressLineOne.

This will also happen if I do late binding with validator.validate(). On a side note how do I know if validation is good or bad? I don't want to continue with submitting a form if the validation is bad. I had to add a isValid property to Validator. or return boolean after the calling the method.

  bind(binding, source, configuration) {
    let targetProperty;
    let target;
    let reporter;
    targetProperty = this.getTargetProperty(binding);
    target = this.getPropertyContext(source, targetProperty);
    this.addRule(target, targetProperty, configuration);
    reporter = this.getReporter(target);
    reporter.subscribe(errors => {
      let relevantErrors = errors.filter(error => {
        return error.propertyName === targetProperty;
      });
      this.renderer.renderErrors(binding.target, relevantErrors);
    });
    binding.target.onblur = function() {
      let config = metadata.getOrCreateOwn(validationMetadataKey, ValidationConfig, target);
      config.validate(target, reporter, targetProperty);
    };
  }

I will test this without a deep field. I will test a field from EnterLocation

jsobell commented 8 years ago

This is always going to be an issue with model identified validation. The information is correct, because it is indeed a property called addressLineOne, but that's not what you want to know. You want to know which object instance it is relative to your display. My POC has the validator in a surrounding DOM element, not in the VM. If the VM wants it (which it often does), you can bind it. Each input with a validate attribute searches back up the DOM elements and finds its parent validator, and registers itself so when you call validator.validate() it can ask each field to perform its own validation, and each field registers success or failure in the validator so the VM or bindings can check to see if a 'field' is valid based on its ID. This also allows me to add custom rules to the validator, but my current POC does not detect changes to dependencies in those rules, although they are triggered by the validate() method. I think the use of bindings to validate is always going to cause issues. It's perfectly valid and useful as a business rules validation solution, but not useful as a visual validation solution. Is this a strong hint that we should have aurelia-validation and aurelia-rules?

y2k4life commented 8 years ago

The only place I can see what I'm looking for, but more than likely not the right way. The closest is let x= binding.target.attributes.getNamedItem('value.bind'); but then I get the other stuff with that & required.

Yup as suspected, I add field, testField1 to EnterLocation and it works. But when I add testField2 to EnterLocation I get a different issue. When I leave testField1 and it is blank I get a message that it is required. When I leave testField2 and leave it blank the message for testField1 is removed and I get a message for testField2.

y2k4life commented 8 years ago

Almost there, I have everything working except for trying to find the correct target and targetProperty. This is an issue regardless of the desired effect of validating upon blur. @PWKad is there an answer to this problem or as @jsobell states that this will always be an issue?

@PWKad as you stated this might not be a desired feature and now I'm working on adding a parameter to the Binding Behavior that makes this optional. validateOnBlur.

I had to make errors in ValidationConfig global. I then added for when results of the validation is good to look for existing error and if one exist to remove from the global errors.

  validate(instance, reporter, key) {
    this.__validationRules__.forEach(rule => {
      if (!key || key === rule.key) {
        let result = rule.rule.validate(instance, rule.key);
        if (result) {
          this.errors.push(result);
        } else {
          let existingIndex = this.errors.findIndex(error => error.propertyName === key);
          if(existingIndex > -1) {
            this.errors.splice(existingIndex, 1);
          }
        }
      }
    });
    reporter.publish(this.errors);
    return this.errors;
  }
y2k4life commented 8 years ago

@PWKad Got it all to work (in theory)! Both in-line validate as you go and validate from the view-model.

I had to get the binding path model.currentLocation then pass that to getPropertyContext. This will also work if there is no deep property. I had to rewrite getPropertyContext. I know there might be issues with my implementation. I got ride of getContextFor and could not make that work with what I was doing. This is a brute force, get it to work idea. Even though I got it to work there are some issues with the blur not being triggered all the time. Might change that to onChange. There are also errors when submitting the form and if an error is already present there is no check and the error is duplicated. I also have issues with inputs that never trigger a blur but don't show when validator.validated is call from the view-model.

I just need to iron out the bugs in all the code in my posts, but the changes work as a proof of concept and might need some enhancements.

  getTargetBinding(binding) {
    let targetBinding = '';
    if (binding.sourceExpression && binding.sourceExpression.expression) {
        let child = binding.sourceExpression.expression.object;
        while (child) {
            targetBinding = child.name + targetBinding;
            child = child.object;
            if (child !== undefined) {
                targetBinding = '.' + targetBinding;
            }
        }
    }
    return targetBinding;
  }
  getPropertyContext(source, targetBiding) {
    var bc = source.overrideContext.bindingContext;
    let bindings = targetBiding.split('.'); 
    let cbc = bc;
    bindings.forEach(binding => {
        cbc = cbc[binding];
    });
    if (cbc) {
        return cbc;
    }
    else {
        return bc;
    }
  }
pfurini commented 8 years ago

Hi, anyone is working on this? Just tried to integrate this plugin in my app, but the UX is really bad, at the point to be unusable, if this issue is not resolved. Great user experience should be our top priority, and as others said it's not clear what the priorities for this plugin are..

EisenbergEffect commented 8 years ago

There's a critical feature being worked on now that should address the basic architectural concerns and enable things to work correctly. It's coming soon.

pfurini commented 8 years ago

👍 great.. but, if I may ask, would you mind to share an expected time frame for that?

EisenbergEffect commented 8 years ago

I hope within the next week, maybe two. @jdanyow Do you have a notion of when your stuff will be ready?

jdanyow commented 8 years ago

@EisenbergEffect things are progressing nicely- my goal is to have something for you and @PWKad to review internally by Sunday or Monday evening. I really want to try and have something ready for public reivew by Tuesday's release if possible. I guess we could do something out-of-band if necessary.

Thanks @apawsey, @stevies, @jsobell, @schuetta92, @jasoncarreira, @y2k4life, @nexbit for all the input. The discussion in this issue has been particularly helpful.

jdanyow commented 8 years ago

Update on validation:

We had a review meeting yesterday. Things are looking good. Some additional changes are being made based on the review. It's looking more like Tuesday the 14th for the release but we should be able to get some sort of preview out before then. To see a recent version of the APIs checkout these repos:

These don't showcase everything, and it's probably tough to tell what the behavior is like by looking at the code but we are definitely targeting all the scenarios mentioned here- @stevies link, the discussion around blur, etc.

y2k4life commented 8 years ago

@jdanyow I have yet to look into the issue below, but I was unable to run the sample. I'm running this on Chrome. I know I know I should wait 'til next Tuesday, just a bit anxious :-) Until then I will just look at the code and not try to run it.

system.js:4 Uncaught (in promise) Error: Super expression must either be null or a function, not undefined
    Error loading http://localhost:9000/dist/bootstrap-validation/bootstrap-form-validation-custom-attribute.js
    at _inherits (http://localhost:9000/dist/bootstrap-validation/bootstrap-form-validation-renderer.js:24:13)
    at http://localhost:9000/dist/bootstrap-validation/bootstrap-form-validation-renderer.js:44:9
    at execute (http://localhost:9000/dist/bootstrap-validation/bootstrap-form-validation-renderer.js:93:8)
    at u (http://localhost:9000/jspm_packages/system.js:5:97)
    at u (http://localhost:9000/jspm_packages/system.js:5:32)
    at Object.execute (http://localhost:9000/jspm_packages/system.js:5:3188)
    at y (http://localhost:9000/jspm_packages/system.js:4:9948)
    at w (http://localhost:9000/jspm_packages/system.js:4:10327)
    at p (http://localhost:9000/jspm_packages/system.js:4:8205)
    at h (http://localhost:9000/jspm_packages/system.js:4:8590)
andrewharry commented 8 years ago

Really looking forward to getting this validation (on blur and submit) figured out. I trust that what you guys come up with will be somewhat more intuitive than the current flavour.

Don't forget to update http://aurelia.io/validation/ :)

andrewharry commented 8 years ago

@y2k4life i'm getting the same 'super' error message

EisenbergEffect commented 8 years ago

Yes, we have a better solution. Also, we'll be taking that site down...it was never supposed to be there actually. A former team member published that without really asking and the way that github works, it appeared as part of our domain.

stefan505 commented 8 years ago

@EisenbergEffect I'm slap in the middle of hooking up validation.Was using aurelia-validation, but I'm having trouble using it with all the latest Aurelia libs. Will the updated solution still land the 14th as mentioned above?

jdanyow commented 8 years ago

this will ship tomorrow (14th) :shipit:

jdanyow commented 8 years ago

shipped! http://blog.durandal.io/2016/06/14/new-validation-alpha-is-here/