Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

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

Closed
dead-claudia opened this issue Oct 1, 2017 · 6 comments
Labels
Area: Core For anything dealing with Mithril core itself Area: Documentation For anything dealing mainly with the documentation itself Type: Breaking Change For any feature request or suggestion that could reasonably break existing code Type: Enhancement For any feature request or suggestion that isn't a bug fix

Comments

@dead-claudia
Copy link
Member

dead-claudia commented Oct 1, 2017

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 dead-claudia added the Type: Enhancement For any feature request or suggestion that isn't a bug fix label Oct 1, 2017
@dead-claudia dead-claudia added this to the 2.0.0 milestone Oct 1, 2017
@spacejack
Copy link
Contributor

spacejack commented Oct 1, 2017

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

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

@dead-claudia
Copy link
Member Author

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

@pygy
Copy link
Member

pygy commented Oct 3, 2017

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 👎

@dead-claudia
Copy link
Member Author

@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.

@lescientifik
Copy link

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.

@dead-claudia dead-claudia added Area: Documentation For anything dealing mainly with the documentation itself Type: Question For issues that are purely questions about Mithril, not necessarily bug reports or suggestions labels Oct 4, 2017
@dead-claudia dead-claudia removed this from the 2.0.0 milestone Nov 10, 2017
@dead-claudia
Copy link
Member Author

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 dead-claudia added the Type: Breaking Change For any feature request or suggestion that could reasonably break existing code label Dec 26, 2019
@dead-claudia dead-claudia added Area: Core For anything dealing with Mithril core itself and removed Type: Question For issues that are purely questions about Mithril, not necessarily bug reports or suggestions labels Sep 2, 2024
@MithrilJS MithrilJS locked and limited conversation to collaborators Sep 2, 2024
@dead-claudia dead-claudia converted this issue into discussion #2943 Sep 2, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
Area: Core For anything dealing with Mithril core itself Area: Documentation For anything dealing mainly with the documentation itself Type: Breaking Change For any feature request or suggestion that could reasonably break existing code Type: Enhancement For any feature request or suggestion that isn't a bug fix
Projects
None yet
Development

No branches or pull requests

4 participants