MithrilJS / mithril.js

A JavaScript Framework for Building Brilliant Applications
https://mithril.js.org
MIT License
14.02k stars 925 forks source link

Allow m('') to produce no element instead of <div> ? #723

Closed maranomynet closed 8 years ago

maranomynet commented 9 years ago

(i.e. support conditional wrapping)

It would be really nice to have some token ("" and/or null seem logical) that produces no element and simply renders its child-nodes and returns them as an array.

Edit: A bit like this utility wrapper:

var mx = function (elm) {
  var vdom = m.apply(null, arguments);
  return (elm !== '') ? vdom : vdom.children;
};

Example use cases:

m('li',
    // Wrap unavailable items in <del>
    m( (item.available ? '' : 'del'),
        m('img', { src:item.picture, alt:item.caption }),
        m('strong', item.label)
        // more markup...
    )
)

Attributes would be ignored for 'non-elements'

m('li',
    // Conditional  link wrapper
    m( (company.url ? 'a' : ''), { href:company.url }
        m('img', { src:company.picture, alt:'' }),
        m('strong', company.label)
        // more markup...
    )
)
pelonpelon commented 9 years ago

If you are suggesting that a parent not be rendered and its children be rendered in an array with no parent -- that seems antithetical to the nature of the DOM tree. Children need a home.

If you still need this done, you can always move your array of children to a new parent after the DOM is rendered with config then remove the parent, all with javascript.

var newParent = document.getElementById('new-parent');
var oldParent = document.getElementById('old-parent');

while (oldParent.childNodes.length > 0) {
    newParent.appendChild(oldParent.childNodes[0]);
}
oldParent.parentNode.removeChild(oldParent);
maranomynet commented 9 years ago

I'm merely suggesting that instead of generating a virtual element, containing its children, to return them in an Array which then gets merged/flattened (like normally would happen with Arrays) into the parent's list of child-nodes.

Same limitations would apply to this and with Arrays currently, and this behaviour can already be implemented with a wrapper function.

var mx = function (elm) {
  var vdom = m.apply(null, arguments);
  return (elm !== '') ? vdom : vdom.children;
};

Making it part of core would be a cheap way to add convenience and improve readability of templates.

barneycarroll commented 9 years ago

My argument against this is that the semantics aren't at all obvious. Given that selectors without tagnames are assumed to be divs eg m( '.hi' ) // => { tag : 'div', attrs : { className : 'hi' }, ... }, there are plenty of places where I (for one) use an empty string simply because in heavily-scripted applications, tagnames and selectors aren't that important. And I use it to mean, 'just give me an element': m( '', { attrs }, ...children ) (In fact, I've previously asked for this behaviour to be extended to invocations where the string as first argument is left out altogether)

I also agree with @pelonpelon in saying that swapping out a level of a DOM hierarchy is extremely unusual – for one it's going to make the structure unreliable which isn't great from a diffing perspective either – but that doesn't mean you shouldn't be able to do it :)

It's worth pointing out that in practical terms, both of your situations can easily be accommodated by simply putting in a noop element – span or div depending on desired display mode:

m('li',
    m( (item.available ? 'span' : 'del'),
        m('img', { src:item.picture, alt:item.caption }),
        m('strong', item.label)
    )
)

m('li',
    m( (company.url ? 'a' : 'div'), 
        company.url ? { href : company.url } : {},
        m('img', { src:company.picture, alt:'' }),
        m('strong', company.label)
    )
)

Alternatively, this rather unreadable mess will have the outcome you desire without the need for a Mithril view language transformer – but it's not exactly intuitive ;)

m('li',
    ( item.available ? Array : m.bind( null, 'del' ) )(
        null,
        m('img', { src:item.picture, alt:item.caption }),
        m('strong', item.label)
    )
)

m('li',
    ( company.url ? m.bind( null, 'a', { href : company.url } ) : Array )(
        null,
        null,
        m('img', { src:company.picture, alt:'' }),
        m('strong', company.label)
    )
)

Honestly, I think no-op elements is the best call.

maranomynet commented 9 years ago

No-op elements are often a no-go - and a cop-out workaround for a limitation in the templating engine.

barneycarroll commented 9 years ago

No-op elements are often a no-go - and a cop-out workaround for a limitation in the templating engine.

Wow, that told me! Good luck ;)

maranomynet commented 9 years ago

Sorry if my reply seemed rude. That was totally unintended. :-(

I simply fail to see how skipping a DOM level is in any way unusual or problematic. It's a fairly common requirement and any templating engine/language makes it difficult/kludgy is, IMO, limited in that sense.

lhorie commented 9 years ago

I'm not opposed to the feature, but the current diff engine has limitation wrt to keys that could lead to some unpleasant surprises, so while it's trivial to add this feature, it's not that trivial to prevent an error in something like this:

[
  m(cond ? "a": null, [m("span", {key: 1}, "foo")])
  //... other stuff
  m(cond ? "a": null, [m("span", {key: 1}, "foo")])
]

With that being said, this only accounts for one level of nesting, so maybe it's acceptable to use the same solution as you would for multiple levels of nesting, e.g.

//dealing w/ conditional multi-level wrapping
function error(message) {
  return m("span.error", message)
}

//in view
errors.length != 1 ? m("ul", errors.map(err => m("li", error(err))) : error(errors[0])

//condtional single-level wrapping
function error(message) {
  return m("span.error", message)
}

//in view
err.url ? m("a", {href: err.url}, error(err.msg)) : error(err.msg)
maranomynet commented 9 years ago

Would it be any different with respect to preventing key conflicts from, say, this:

function error(message) {  return m("span.error", { key:1 }, message);  }

// in view:
[
  err1.url ? m("a", {href: err1.url}, error(err1.msg)) : error(err1.msg),
  //... other stuff
  err2.url ? m("a", {href: err2.url}, error(err2.msg)) : error(err2.msg)
]

Shouldn't developers be encouraged to use fairly unique keys anyway?

lhorie commented 9 years ago

Using a function doesn't address the core issue with potential duplicate keys, so you'd still be responsible for making sure there aren't conflicts (at least until I fix the diff engine to support sibling keyed arrays). I just brought up refactorization into a function as a generalization of the use case

l-cornelius-dol commented 9 years ago

I am opposed to this. Maybe with null, but definitely not ""; that's logically at odds with ".xyz" producing <div class="xyz">.

maranomynet commented 9 years ago

Agreed, "" is already taken for impicit "div".

lhorie commented 8 years ago

I believe @barneycarroll was using "" as "div", so I'm gonna leave it as is for now, with the caveat that since it's not documented, the current behavior is undefined behavior

dead-claudia commented 8 years ago

I believe I changed (with @barneycarroll and others being in the conversation on Gitter, IIRC) to just unconditionally throw. It's a degenerate case, and it's full of gotchas, with some expecting m("div") and others expecting no element to be rendered.

On Sun, Dec 20, 2015, 19:23 Leo Horie notifications@github.com wrote:

Closed #723 https://github.com/lhorie/mithril.js/issues/723.

— Reply to this email directly or view it on GitHub https://github.com/lhorie/mithril.js/issues/723#event-496981696.