Extend signature for m.withAttr( attributeKey, callback, shouldRedraw=true )

[barneycarroll]

m.withAttr is one of the cornerstones of the Mithril redraw cycle because it allows simple bi-di binding through forced synchronization. However, there are situations where the callback has conditional logic or VM-only implications and a redraw is unnecessary and possibly undesirable – consider an input field whose content needs validation: the user needs room for error in constructing a valid input, so change is only registered when the user leaves the input (onchange) or hits enter (onkeypress && event.keyCode === 13). The problem is that any withAttr callback on a scale smaller than the window of opportunity (ie onkeypress) forces model synchronization.

[tivac]

I'm fine with this change, but not the proposed API. It falls into the boolean trap of limiting future expansion and being hard to understand when you read it w/o having the API docs handy.

m.withAttr("value", callback, { redraw : false })

Would be a more expressive & future-friendly API for this functionality.

[barneycarroll]

It's only worthwhile if it keeps things legible (as opposed to going off-API) but conveniently terse. At this point I figure I may as well do without it:

m.withAttr( 'value', callback, 0 );
m.withAttr( 'value', callback, { redraw : false } );
function(){ callback( this.value ) };

[l-cornelius-dol]

This might allow me to solve my problem with excessive redraws on key and mouse events.

I think I lean toward the more extensible {redraw:false} syntax, since it can easily be wrapped in an application level function to restore terseness (e.g. withAttrNR for withAttrNoRedraw).

Perhaps the parameter should be the redraw strategy though:

m.withAttr("value",callback,"none")

which helps alleviate the boolean trap with respect to needing documentation.

A caution I would offer is, how well will this interact when there are multiple handlers for the event with conflicting redraw values? In my case a key handler on some container object(s), and a key handler on the page body.

Conceptually, I have a key handler for a list and a key handler for the page. If an ESCAPE key is not actioned by the list handler (say, to cancel an edit), it might be actioned by the page handler. Since these have no knowledge of each other, they use the preventDefault property to defer gracefully and generically. Something like that would have to be true of this redrawing facility such that if any handler explicitly specifies redrawing, it is applied, but only if not already explicitly set or if already explicitly set only allowing a change from false to true.

[barneycarroll]

@lawrence-dol redraw strategy looks really nasty if you ask me. It also makes the mistake of glossing over a lot of possible complication (like the hidden conflict stuff you're talking about) with the appearance of simplicity, which I don't like. How would this help you for a keypress event anyway? It doesn't involve an element attribute – and with key handlers, execution conditions are not met most of the time anyway (a handler for escape will noop whenever any other key is pressed), so it's crazy to suggest you want to change redraw strategy on the event handler (when the outcome has yet to resolve). Let's take this convo back onto the list, I reckon :)

[l-cornelius-dol]

@barneycarroll: I've wrongly conflated the redraw strategy with whether to do startComputation/endComputation around the event handler. With that mistake ignored, I think the rest remains valid, except my entire point of contention disappears. I was mixing the strategy up in this because I had been experimenting with changing the redraw strategy to work-around the fact that the redraw was going to happen for that handler and I can't stop it.

My chief need is to suppress the automatic redrawing for some event handlers, allowing them to explicitly call m.redraw() if they do something that affects the view. As long as multiple m.redraw() calls coalesce that will work great.

So I would support this change.

[barneycarroll]

Also worth noting this proposal misunderstands the auto-redraw mechanism, which triggers as a side-effect of an event being registered via Mithril (I thought it was a side effect of m.withAttr – I really should start reading the code first).

[barneycarroll]

@lawrence-dol and I'd wrongly conflated m.withAttr with the autoredraw mechanism. It seems the best way of going about this 'manage your own redraws' lark would be to go through config. Maybe some kind of helper function could make the pill easier to swallow.

// Mithril non-redrawing event binder for config
function on( eventname, callback ){
    return function( el, init ){
        if( !init ) el.addEventListener( eventname, callback, false );
    };
}

// Utility function for handling only certain keys
var onKey = ( function(){
    var keymap = {
        'enter' : 13,
        'space' : 31,
        'tab'   : 9,
        'esc'   : 27,
        'left'  : 37,
        'up'    : 38,
        'right' : 39,
        'down'  : 40
    };

    function noop(){}

    return function onKey( keyCode, success, failure ){
        keyCode = keymap[ keyCode ] || keyCode;
        failure = failure           || noop;

        return function handler( event ){
            ( event && event.keyCode === keyCode ? success : failure ).call( this, event );
        };
    };
}() );

// In your view
m( 'input', {
    config : on( 'keypress', onKey( 'esc', killModal ) )
} );

// However you want to handle redraw is up to you
function killModal(){
    doStuff();
    m.redraw();
}

[lhorie]

I'd agree that a helper would be better than conflating withAttr and redraw strategy

Redraw strategy and event handlers in config fill overlapping responsibilities, with the difference that template events (e.g. {onclick: ...} auto-redraw by default, and config events don't auto-redraw by default.

Changing the default for config redraws is easier to reason about because adding functionality is easier than removing, but template events are less boilerplate-y and thus more convenient.

For key events, you could do something like

function withKeys(options) {
  return function(e) {
    //simplified key map, todo: enter, esc, space, etc
    var key = String.fromCharCode(e.keyCode)
    var callback = options[key]
    if (typeof callback == "function") {
      callback(key)
    }
    else m.redraw.strategy("none") //if key is not in map, ignore this event
  }
}

//this top onkeypress triggers redraw regardless of redraw strategy in the inner keypress
//you can, of course, use m.redraw.strategy to turn off redrawing for it as well
m("div", {onkeypress: function() {console.log(1)}}, [
  //the inner keypress only redraws if key is a or b
  m("div", {onkeypress: withKeys({a: doFoo, b: doBar})})
])

@barneycarroll's alternative should also work well, but I'd recommend using m.startComputation / m.endComputation unless you explicitly want a synchronous redraw even if you have ajax requests happening in the callback

[l-cornelius-dol]

While I agree with the fact that withAttr is wrong, and that this particular issue should be closed, I think that the overall idea of having a really convenient way to choose whether the autoredraw for events should be done is valid.

However, it can't be a global "don't auto-redraw for any event handler", because that will be a long term nightmare, making self-contained components impossible due to unforseen interactions.

But I feel strongly that there needs to be some really simple, boilerplate-free way of adding an event handler without triggering redraw. I am going to consider this and possible open another issue.

Certainly, I've tried turning off redraw with changing the redraw strategy, and it turns out to be practically impossible to get right, and very error prone in the face of ongoing enhancements (meaning, even when you get it right, it's easy to break with code added later). This is principally because although one component's event handler doesn't require a redraw, another's might and they can't reliably change the strategy independently -- they could if the default was to not auto-redraw, since anyone could turn redraw on if the conditions are met, but no one can turn it off since the redraw requirement is an "or" condition with respect to the event handlers. And if the default were not to redraw, it would just be a case of calling m.redraw() when the conditions requiring a redraw were met.

Since self-contained components are crucial for the survival of Mithril or any framework (written in-house currently, but externally in the long term), Mithril must make components possible. For example, it must be possible to write a date-picker for Mithril which anyone can drop into their project with no conflicts.

What about having a "noredraw" object in the m configuration which contains handlers which don't get wrapped with start/end computation?

// this top onkeypress triggers redraw regardless of redraw strategy in the inner keypress
m("div", { onkeypress: myKeyHandler }, [
    // the inner keypress only redraws if key is a or b
    m("div", { noredraw: { onkeypress: withKeys({a: doFoo, b: doBar}) } })
])

Indeed, if I had my choice, with my (admittedly limited) experience so far, I'd rather have an autoredraw { ... } for the current behavior and have event handlers not autoredraw by default.

[l-cornelius-dol]

@lhorie: Is the only current value-add for start/end computation in working with async AJAX? Because I've found (in agreement with Barney), that in any but the most trivial project all AJAX requests must be background:true and therefore trigger redraw on completion, since the UI must absolutely remain active while requests are pending.

IOW, I have zero use for halting redraw while AJAX requests are pending, since it allows for no feedback to the user that the request(s) are pending. Nothing is more annoying than a UI when it appears "nothing happened" for a few seconds after you click something. And that was my first, biggest problem when starting with Mithril, getting it to show the user that a network request was in progress. I had to "work-around" Mithril's default behavior to do the obvious best thing for my user, and the first thing I did was wrap m.request to force background:true as a default for all requests.

Worse, if you ever end up with a start not paired with an end, your UI is hosed (frozen). Start/end computation seems extremely error prone to me, and, frankly, I suspect it should be dropped. What's the value-proposition in favor?

[lhorie]

I may be misunderstanding the use case, but what is the value of not autoredrawing by default?

Even if you changed the defaults of whether a redraw occurs on an event, you can't predict what triggers redraws in other parts of the system, and redraws are always global. Because of that, you should never consider redraw prevention as a mechanism to make your UI look the way you want. In other words, if the UI looks different before and after randomly calling m.redraw() from console, then there's a design bug.

m.redraw.strategy("none") is supposed to be a performance optimization for when you know for certain that a specific event didn't affect the data model; it's not meant to be a mechanism for deliberately de-syncing UI from data.

If you are trying to deliberately de-synchronize the UI from the data model, then you need to understand that you're introducing transient state (i.e. state that isn't tracked at the model/view-model level) and, for that, you probably should be using config. It has a context argument precisely for storing cross-redraw state that you don't want to be part of the data model.

What you can't have is state that is sometimes transient and sometimes not (and have these semantics have a terse syntax at the same time), because you have to manually specify what are the conditions that make the state transient.

Re: start/endComputation: The reason start/endComputation exist is to allow asynchronous services to integrate to the redraw-deferring mechanism. If you're writing your own helpers and you're always force-redrawing within your codebase, then you can get away with not using start/endComputation, but if you're writing a library, you should consider that consumers of that library may expect the same autoredrawing semantics as template events (in which case, you should use start/endComputation)

A more performant way to go about loading icons specifically is to drop down to raw DOM manipulation for showing/hiding loading icons (since these are local, self-contained, predictable DOM interactions) and let redraws only happen when the data is guaranteed to be resolved.

[l-cornelius-dol]

@lhorie

what is the value of not autoredrawing by default?

Well, simply binding an event handler is too coarse to determine if redrawing should happen. The case-in-point in this thread is a key handler. The fact that a key event occurred means nothing; it is only if that key event causes something in the model to change. I haven't thought it through deeply, but this seems a general case for all events. The fact is, a redraw should not occur for every key typed just because I am trying to take special action on ESCAPE and ENTER. Nor, and this is my opinion, should I have to go to special lengths to register such a handler.

Ergo, there should be an easy way to not auto-redraw for any specific event handler, but to defer that decision to the specific event handler. And that does not have to be with m.redraw, it might just as well use start/end computation:

function keyEvent(evt) {
    if(evt.keyCode==13) {
        m.startComputation();
        // do stuff
        m.endComputation();
        }
     }

Using m.redraw.strategy() is no good for this, because currently redrawing will happen unless suppressed; but the event handler doesn't know whether redrawing can be safely suppressed (it may be needed for other reasons), it knows only if redrawing should be done because it effected a change in the model. I know, because I tried the m.redraw.strategy() route and it utterly fails.

None of this is to say that some other event handler may not auto-redraw for the same event, either automatically or manually. Only that it should be possible to tell Mithril, "don't auto-redraw simply because this handler was invoked", because the handler may end up doing nothing to the data models. I strongly suspect that key and mouse events should never autoredraw, because they only may or may not affect the currently rendered UI.

If you are trying to deliberately de-synchronize the UI from the data model

No, that's not it at all. It's that the fact that the model has changed somewhere isn't true simply because an event handler ran, but because the event handler actually changed the model. So it's the opposite -- I want this capability because my models entirely drive my views.

The reason start/endComputation exist is to allow asynchronous services to integrate to the redraw-deferring mechanism.

That may be true in some cases (I suspect maybe only trivial examples?), but in my experience asynchronous services are _entirely_ orthogonal to rendering. It's my opinion that the render must be able to happen at any moment, without regard to any async requests, and the the models must be internally consistent at all times such that a render always produces intelligible results. Barney has in other thread(s) pointed out that the synchronous examples are toy cases unrelated to real-world apps.

In this respect, I think that Mithril currently gets it wrong (and I am fairly certain of this). Async requests should be background by default, and triggering redraw at the completion of the request should be the responsibility of the request success/failure code.

Note that there does need to be a new m.redrawNeeded() (or some such) function that only triggers redraw if pendingRequests is zero, which is actually what I have meant with every use of m.redraw() to date. Right now this can be done with a helper that just calls start/end, but that's indicative of the fact that there's a third flavor of redraw signalling, currently not provided by Mithril, which is "the model has been altered, so redraw when drawing is next activated".

more performant way to go about loading icons specifically is to drop down to raw DOM manipulation

But that's no good for background requests, because a render may happen for other reasons before the async request completes (and it will for any non-trivial app), blowing away my loading icon, so the view-model must reflect the fact that a request is pending and cause that that fact to represented in the UI while the request is pending. Worse, in the context of Mithril diving into the DOM is kludgy and non-idiomatic.

And if rendering is blocked, then the app is already broken because UI should never block for arbitrary periods of time, especially not on notoriously unpredictable network access.

In other words, using "foreground" AJAX is a fundamentally broken idea.

PS: All of this is just my 2 cents; I could be entirely wrong, but I think I am at least partially or mostly correct.

PPS: I love Mithril, and am totally sold on the general concept, or I wouldn't care enough to belabor the point.

[lhorie]

Ok, this is getting a bit off-topic (as far as the original title was), but anyways.

I think we should continue the discussion re: a default-to-no-redrawing event handler in #378.

Re: foreground/background ajax: one of the reasons background: true is not default is that it and the defaultValue option (which is somewhat crucial for it to work reliably) did not exist until fairly recently.

The original rationale for foreground-by-default had primarily to do with performance in non trivial apps (as in, apps that trigger several requests from a single event handler). Making background:true default for all requests means that you'd be running as many redraws as there are requests. This was one of the unfixable facets of a very complex performance problem that I had on a very-non-trivial Angular app at my previous job.

The overall context was that slow requests were something we could always fix with better SQL queries, but needless spam-redrawing was something that we could not control in Angular. IMHO, compensating for slow SQL by making UI rendering incremental (and slower as a result) is a band-aid solution at best, but I digress.

The other reason for foreground-by-default is that there's no way to set a reasonable defaultValue automatically because you need to know the type of the response object ahead of time. Because of this, there's no way to automatically guarantee even a minimum level of type safety required for templates to render with unresolved data.

Nowadays, we can make requests background-by-default (as you have done), but it still requires setting appropriate defaultValue's for each request manually.

At this point, I'd say the argument for foreground-by-default is that not having to specify a defaultValue is easier on newbies. I think this is somewhat significant because understanding the need for defaultValue requires understanding the redrawing system, which is a farily complex abstraction (and being an abstraction, it should ideally be invisible until you reach advanced user level).

[l-cornelius-dol]

I agree, and this issue should be closed now.

Ironically, it's exactly the inability to prevent spammy redraws caused by event handlers that caused Barney and I to raise this in the first place.

It's worth reiterating that as a newbie the foreground-by-default caused me the most problems. In the end it was quite intuitive to default the model data and update it when the request completes.

[lhorie]

Ok closing this