Refactor renderer interface

[jsobell]

How do we provide custom error messages on a per-field basis? This is particularly important for rules such as @format Also, any custom message rule should inject the current options and value. In treacherous we have .withMessage that takes either a string or a function with options and value passed in as parameters:

.forProperty("uRLAlias").addRule("launchURL",null).withMessage((v,o) => `${v} is not a valid launch URL`)
.forProperty("exitURL").addRule("regex", url).withMessage((v,o) => `${v} is not a valid exit URL`)
.forProperty("pauseURL").addRule("regex", url)

Perhaps @validationMsg can be used as a decorator, or/and

@format({ message = (v,o) => `${v} is not a valid exit URL`})

[taz]

It appears that, at least for the rules applied via a decorator, that an object can be passed in that is subsequently passed back to validate.js on a per-field basis.

ie: @email({message: '^Hmm %{value} is not a valid email address'}) email;

I'm not sure to what extent this applies for the other rule types however, and it uses validate.js's interpolation language, not Javascript/Aurelia's (% vs $ etc).

That works for fairly static messages, but I haven't worked out how to pass something more advanced (like for example an i18n key) in as yet.

[taz]

It also doesn't seem to update the %{value} if the field is changed from one invalid state to another... It does if you completely blank the value, blur the field and then enter another invalid value. I suspect this is de-triggering and re-triggering the validation rule and as such, re-evaluating the string.

[taz]

Worked out @format as well...

https://validatejs.org/#validators-format

@format({message: "^Bad Format", pattern: "[a-z0-9]+"}) fieldName;

[taz]

@jdanyow I can't speak for @jsobell but I still have a concern re: the best way to update %{value} when the user changes the input (and it remains invalid).

Is it possible to force validate.js to do an update of the error text when the model changes somehow?

[jdanyow]

ok- re-opening, please use the gist from yesterday's blog post to create a sample of what you're trying to achieve and we can look into it.

[taz]

Edit: Ignore that, got my issues mixed up! That was a response for #79! I'll create another...

[taz]

https://gist.run/?id=9beab195fbe5321639f1c2536e8d21a3

Step 1: Enter 'test@invalid' as the email address. Result 1: "Nope test@invalid is not a valid email address"

Step 2: Edit the email address, make it 'test@still_invalid' Result 2: "Nope test@invalid is not a valid email address" Expected Result: "Nope test@still_invalid is not a valid email address"

Note that switching to validateTrigger.change makes no difference (which incidentally is the mode i left the gist in). Once the error is displayed, it never re-renders it while it's in an error state...

Bonus Points: Action: Enter 'test@invalid.domain.tld.y?' Result: "Nope test@invalid domain tld y is not a valid email address" ... why aren't there any periods in the email address anymore? :-) I'm going to assume that's possibly a problem on validate.js' side of the fence?

Thanks,

B.

[jdanyow]

@bblazely I think this is a problem with the ValidationController's logic.

https://github.com/aurelia/validation/blob/master/src/validation-controller.js#L103

here's what's happening:

1. data entry in the input causes the validate decorator to notify the controller (either on blur or immediately).
2. the controller asks the validator to validate the binding's current object+property combination
3. the validator returns an array of errors
4. the controller uses the new array of errors in conjunction with the bindings existing set of errors and determines which errors need to be unrendered and which need to be re-rendered. This is where the problem is. The current logic says "have I already rendered this error", if yes, it skips it. The check is based on the rule associated with the error. So even though the new error has a different message, it's the same broken rule, so it's skipped by the current logic.

I think the logic needs to change to "unrender all existing errors and render all new errors", so that the message stays up to date.

[taz]

@jdanyow just woke up, will take a look at this in about an hour. Only challenge with this approach may be when the display of the error message is animated... Which I've implemented in my test app :)

[jdanyow]

@bblazely this fix hasn't been published yet- you'll need pull down the repo and run npm install;gulp build and copy the contents of the dist/amd folder into your project if you want to try it right away

[taz]

Yep, no worries

[taz]

@jdanyow Ok, I monkey patched the changes in and it behaved pretty much exactly as I expected unfortunately.

http://i.imgur.com/pbEM4Uj.gif

I think that maybe in addition to unrender/render there may need to be an updaterender (bad name?) call in there instead, that would look more like the original _updateErrors method, but instead of ignoring errors it thinks it's already rendered, it pushes them to an updateRender method.

EDIT: I've been examining the code and I see why this is potentially quite painful to do now. Mulling it over...

That method could then optionally locate an existing error span (up to the developer to work that bit out) and replace the content / shake the error / play a sound / whatever.

Maybe if the method isn't specified in the ValidationRenderer it can default to a simple unrender/render action?

I've also been mulling over how I'd manage more complex messages and I don't think the build in 'message' field in the rule is going to cut it. Particularly for messages that require access to the model, function calls or fields that present a 'severity' of error (ie: error/warn/ok). I might need to log a new job for that though...

I'm porting an existing login form from Angular over to Aurelia as an exercise for this, so I'm coming up with quite a few use cases. :-)

[taz]

@jdanyow Also, related to this, one of the errors i'm porting from the 'other' framework looks like this:

"HELP_NOT_FOUND":      "Account not found! <a href='#' ng-click='view.change(VIEW.SIGN_UP)'>{{'SIGN_UP' | translate}}</a>?"

Which will become:

 "HELP_NOT_FOUND":      "Account not found! <a href='#' click.delegate='view.is = view.sign_up' i18n='login:SIGN_UP'></a>?"

I'm not sure how I'm going to manage this yet in the context of a custom message. It requires both an i18n substitution and access to the model to change the current 'view' state of the component.

Worth submitting a separate issue about it?

[taz]

So, bindings have a _version? Can that be leveraged?

let target_changed = false;
 if (binding.last_version === undefined || binding.last_version < binding._version) {
        binding.last_version = binding._version;
        target_changed = true;
    }

this._updateErrors(errors, newErrors, target, changed);

The more I think about it though, the more I feel that the original code would be fine if the custom error messages were somehow bound to the model for updates, not generated by validate.js.

That would (potentially) solve i18n, the use case above, and this issue, wouldn't it?

EDIT: Maybe a workaround? In the bootstrap ValidationRenderer this.templatingEngine.enhance({'element': message, 'bindingContext': error.object});

EDIT2: Nope, Sigh.This causes strange results when in manual/blur mode though without some sort of change debounce... :-)

[taz]

Ok, I think this mostly works as a proof of concept...

Incorporates updates (not elegant, but they work - I think) and model binding in the 'message'.

I haven't tested event delegates or i18n, but i think they should work too... maybe...

Monkey Patch to ValidationController:

ValidationController.prototype._updateErrors = function(errors, newErrors, target, targetValueUpdated) {
    let i = 0;
    while (i < errors.length) {
        const error = errors[i];
        const index = newErrors.findIndex(x => x.rule === error.rule);
        if (index === -1) {
            errors.splice(i, 1);
            this._unrenderError(error, target);
            continue;
        } else if (targetValueUpdated) {
            this._renderErrorUpdate(error, target);
        }
        newErrors.splice(index, 1);
        i++;
    }

    i = 0;
    while (i < newErrors.length) {
        const error = newErrors[i];
        errors.push(error);
        this._renderError(error, target);
        i++;
    }
};

ValidationController.prototype._renderErrorUpdate = function(error, target) {
    var renderers = this.renderers;
    var i = renderers.length;
    while (i--) {
        if (renderers[i].update) {
            renderers[i].update(error, target);
        } else {
            renderers[i].unrender(error,target);
            renderers[i].render(error, target);
        }
    }
};

ValidationController.prototype._validateBinding = function(binding) {
    let updated = false;
    const { target, rules, errors } = this.bindings.get(binding);
    const { object, property } = getPropertyInfo(binding.sourceExpression, binding.source);
    const newErrors = this.validator.validateProperty(object, property, rules);

    if (binding.last_version === undefined || binding.last_version < binding._version) {
        binding.last_version = binding._version;
        updated = true;
    }

    this._updateErrors(errors, newErrors, target, updated);
    return errors;
};

Changes to the bootstrap renderer:

  render(error, target) {
        if (!target || !(this.boundaryElement === target || this.boundaryElement.contains(target))) {
            return;
        }

        // add the has-error class to the bootstrap form-group div
        const formGroup = target.querySelector('.form-group') || target.closest('.form-group');
        formGroup.classList.add('has-error');

        // add help-block
        const message = document.createElement('span');
        message.classList.add('au-animate', 'help-block', 'validation-error', `error_${error.rule.name}`);
        message.innerHTML = error.message;
        message.error = error;

        formGroup.appendChild(message);
        this.templatingEngine.enhance({'element': message, 'bindingContext': error.object});
        this.animator.enter(message);

    }

    update(error, target) {
        if (!target || !(this.boundaryElement === target || this.boundaryElement.contains(target))) {
            return;
        }
        const formGroup = target.querySelector('.form-group') || target.closest('.form-group');
        let message = formGroup.querySelector(`.error_${error.rule.name}`);
        if (message) {
            message.innerHTML = error.message;
            this.templatingEngine.enhance({'element': message, 'bindingContext': error.object});
        }
    }

    unrender(error, target) {
        if (!target || !(this.boundaryElement === target || this.boundaryElement.contains(target))) {
            return;
        }

        // remove the has-error class on the bootstrap form-group div
        const formGroup = target.querySelector('.form-group') || target.closest('.form-group');

        // remove all messages related to the error.
        let messages = formGroup.querySelectorAll('.validation-error');
        let i = messages.length;
        while(i--) {
            let message = messages[i];
            if (message.error !== error) {
                continue;
            }
            message.error = null;
            this.animator.leave(message).then( () => {
                message.remove();
            });

            if (i == 0) {
                formGroup.classList.remove('has-error');
            }
        }
    }

Test Email Rule:

    @required({message: 'Required'})
    @length({message: 'Length Required!', minimum: 5})
    @email({custom: 'did this work?', message: '^Nope! ${email & oneTime} is not a valid address'})
    email;

I think that's everything I changed...

I'll try and patch it all into a GistRun after lunch so you can see it running.

B.

[taz]

@jdanyow

https://gist.run/?id=bc6bf040204b1e80e13ba57f8244cd58

• i18n bindings seem to work (not in the gist though)
• I disabled the animation hooks from the renderer - couldn't get gistrun to include them :(
• bindings to the model seem to work too.

Concerns:

• Potentially confusing mish mash of validate.js and aurelia bindings are possible...
• May need to introduce a "requiresUpdateOnChange" type attribute to the rule definition to eliminate extraneous updates on error messages that don't need it.
• Doesn't 'feel' elegant. (Especially in the renderer)
• May have introduced a bug when using if.bind (errors aren't cleared when if-bind='false'). Oops, found it, used old code somehow!

[jdanyow]

@bblazely initially the controller wasn't telling the renderers to unrender and re-renders if it wasn't a "new" error (an error for the same rule had already been rendered). This caused problems with folks whose error messages were dynamic. Now the controller always unrenders all the previous errors and renders all the new errors, which is bad for animation scenario as you've shown.

I think we might need to change the renderer API to give developers complete freedom to handle the error changes:

- renderError(error: ValidationError, target: Element = null)
- unrenderError(error: ValidationError, target: Element = null)
+ updateErrors(
+   added: ValidationError[],
+   removed: ValidationError[],
+   updated: ValidationError[],
+   target: Element = null)

This should give the render implementor complete freedom to handle the DOM changes.

What do you think? I'm going to run this by @EisenbergEffect too- he'll have some ideas.

Please open a separate issue for error messages with html content. My initial thought is it would be the responsibility of the renderer to use enhance to compose the markup.

[EisenbergEffect]

@jdanyow I've been lurking and watching the conversation. The proposal you have makes sense to me. I didn't follow the reference to enhance though. Can you expand on that use case?

[taz]

@jdanyow

Your suggestion is certainly cleaner and feels more elegant, but I think there's still a case where the renderer will need to know to update the error shown outside of old / new.

For example, lets say you have a login form and the user gets the password wrong. The system imposes a 5 attempt limit. Each time, you want to tell the user that they have 1 less attempt before you lock them out.

In this scenario, on each attempt the field stays "in error", so oldErrors isn't populated and "newError" was only populated on the first one.

The latest approach of adding/removing the error each time would work IF you could tie an old error to a new error in the renderer somehow I suppose - i.e. the renderer could just do an update without triggering an animation. I assume that would work much like the _version tracking code I added to the gist. But is that complicating the renderer? To me that seems like something that should be done once by the validation layer, not implemented each time in the renderer... Happy to be convinced otherwise...

The example with the email address in the gistrun that I posted above is a good (if contrived) example too. I'm using the value of the email field in the message. It's a oneTime bind (otherwise it updates the message while i'm typing, which is just bizzare to watch), so it needs to know to re-render upon validationTrigger. It shouldn't have to go to the trouble of removing and re-adding an element to the DOM in my opinion.

re: HTML / enhanced content, I agree with that. Will open a new topic to discuss further.

Hope that all makes sense, it's after midnight and my daughter woke up twice while i was writing it, so I lost my train of thought. :-)

[taz]

@EisenbergEffect The enhance reference in the Renderer was to allow the error message to influence the model. Edit: And render HTML :-)

You can see it used in the gistrun a couple of posts up, put in an invalid email address in the "Email Constant" field and click the link you get from the error message.

I've used this method in the past to redirect a user to a sign-in state if the username was taken during sign up etc.

[taz]

It also handily lets me specify an i18n="" binding, but hopefully there'll be a better way to do that at some point :-)

[EisenbergEffect]

Could a simple string template replace bet a better option than invoking the compiler for each error? The syntax could be made to work the same. Maybe instead of using enhance it could even just use the syntax parser to parse the string into a binding expression and then eval it against the context. @jdanyow Something interesting to think about at some point. It would be nice to have a better way to handle this than a full up view compile.

[taz]

I've opened a new issue for dynamic/enhanced messages as #90 so that it doesn't get muddled in with this one.

I used enhance initially as I'm not familiar enough with Aurelia yet to know of another way to do it.

[taz]

@jdanyow Looks like you did an edit on your proposal while I was upstairs putting my daughter back to bed. I only just saw it now sorry - I did mention it was late? :-P

+updated <-- This! Much better. I couldn't find a good way of grabbing it per-validator though, do you have some magic?

If you can implement added/removed/updated, that would do the trick I reckon.

[jdanyow]

renderer interface was refactored