mithril.js icon indicating copy to clipboard operation
mithril.js copied to clipboard

Suggestion: allow Hyperscript selectors to be used in the diffing algorithm

Open dead-claudia opened this issue 6 years ago • 6 comments

Currently, people are getting confused by cond ? m(".foo") : m(".bar") not diffing as expected. It's come up quite a few times in Gitter by now, and it leads to some pretty subtle user bugs.

Expected Behavior

What many people are expecting is that m(".foo") and m(".bar") are treated as different elements, and I'm suggesting using that as part of the node's identity, as if it were part of the node's tag name.

Current Behavior

Currently, Mithril sees m(".foo") as no more than m("div", {className: "foo"}), and diffs based on the tag name itself.

Possible Solution

Add a check here to also check for the Hyperscript selector along with the tag name heuristic.

Context

It'd avoid subtle bugs like in this, where oncreate doesn't fire like you'd visually expect if vnode.state.loading is initially true.

// Taken from a recent Gitter conversation
vnode.state.loading
    ? m('.content-loader.h-100', m('i.fa.fa-spinner.fa-pulse.fa-5x.fa-fw'))
    : m('.channel-envelopes.layout-main-block.flex.flex-rows.scrollable', {
        oncreate(node) {
            console.log('oncreate')
            vnode.state.scrollingContent = node.dom
        }
    }, vnode.state.envelopes().map(envelope => m(ChannelEnvelope, {envelope}))),

Edit: An alternative to this would be to document the behavior as a common gotcha, recommending the use of key as applicable.

dead-claudia avatar Oct 01 '17 19:10 dead-claudia

Could break some apps if people were relying on the old behaviour...

m('div' + isActive ? '.active' : '', ...)

spacejack avatar Oct 01 '17 20:10 spacejack

@spacejack I'm aware, but it should only present itself if you use oncreate, onupdate, or assume vnode.dom is constant across differing selectors.

dead-claudia avatar Oct 01 '17 21:10 dead-claudia

I'd rather improve the docs than introduce this. Both behaviors make sense, and users can opt into remove/create by using keys if needed. Oh, now that I think of it, I suppose that you could force update by using the same selectors and passing parameters through the attrs object rather than the selector...

Mild :-1:

pygy avatar Oct 03 '17 16:10 pygy

@pygy Just to throw this out there, I'm only mildly 👍 , just the only reason I even suggested it was for UX reasons, not necessarily because I'm otherwise interested in it.

dead-claudia avatar Oct 04 '17 02:10 dead-claudia

I just submitted an issue on snabbdom for the exact inverse problem (using h("div#myID") leading to setting a semi-implicit key on the Vnode).

I like the way mithril is using this syntax only as a shorthand for settings class/id on the element, letting the key property be the only "real" / explicit key.

Anyways, both options seem viable to me, and probably the documentation option is the more conservative/least painful action to take.

lescientifik avatar Oct 04 '17 08:10 lescientifik

Revisiting this months later, I see very little actionable here. Here, it's documented to just be one way to pass attributes, hardly any different from passing dynamic attributes. The docs could use a little clarification that they're equivalent, but this is the only thing necessary.

dead-claudia avatar Jan 17 '19 17:01 dead-claudia