MithrilJS / mithril.js

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

false values in views should be coerced to zero length text nodes #1014

Closed barneycarroll closed 7 years ago

barneycarroll commented 8 years ago

I'm surprised this isn't in while undefined and null are translated — the irony is that an undefined value is more likely to be indicative of a fault in application logic (author expected a value when there was none), but Mithril papers over this, which could lead to frustration as bugs fail silently. But I digress.

Conditional logic is essential to clever views. Even the most restrictive of templating languages (consider Handlebars, used as the basis for Ember, where the prevailing attitude is decidedly against Turing-complete views) implement their own if handlers.

Mithril can't do ifs without resorting to temporary assignment or split returns, which represent an unpleasant deviation into imperative coding in an idiom that favours functional, point-free code.

Ternary operators are always possible but I'm not the first to complain at the ugliness of condition ? content : '' — especially when success content may be voluminous it's inelegant to leave a hanging else, especially when so often the else outcome is just a dangling empty string; even more contrived is !condition ? '' : content, which makes it clearer early on that there is no else content, but introduces a lot of cruft.

The ultimate use case for false virtual nodes outputing empty text nodes is the ability to write condition && content or condition || content for simple ifs without elses.

Consider this stupid fiddle — and then think of non-trivial conditional content (eg a component invocation, like a modal). Wouldn't it be awesome?

lhorie commented 8 years ago

The ultimate use case for false virtual nodes outputing empty text nodes is the ability to write condition && content or condition || content for simple ifs without elses.

But then, it follows that true should also yield a zero length text node, e.g. https://jsfiddle.net/0a8835be/1/ especially since foo || bar is far more intuitive than foo && bar (since "or" has a connotation of "either foo or bar, by process of elimination", but "and" has no such connotation)

I know Angular and PHP have similar behavior wrt to false, but it's more of a dev (mis?-)feature, not something you'd explicitly use for production code.

I'd like to hear what others think of this.

tropperstyle commented 8 years ago

I recently re-started following the mailing list, and of course, the first new thread peaked my interest. Quick background: I use mithril in production to provide native-like experiences. I use npm, browserify, and cordova to target cross-platform iOS and Android apps.

This is what I have settled with:

m('div.container', [
  shouldBeHere() ? [
    m('div.child')
  ] : null,
]),

It's looks alright, albeit verbose.

I did initially attempt this, and still in some places, would prefer:

m('div.container', [
  shouldBeHere() && m('div.child')
]),
tivac commented 8 years ago

I'm on-board w/ false being equivalent to null or undefined, but allowing true to do the same feels a bit strange.

For this use-case I'm not really convinced by Leo's && vs || argument, but that seems like a pretty personal-code-style type of thing. I'd use && all over the place w/ this change and be perfectly happy w/ that.

lhorie commented 8 years ago

Oh, I'm not arguing for making true yield empty text node. I'm just saying I find foo || bar clear, but not foo && bar. I agree that true yielding empty text node is weird.

What I was getting at is that special casing false makes boolean handling inconsistent (such that flipping a conditional during a refactor would cause true to print out where nothing did with a false).

tivac commented 8 years ago

Yeah, the inconsistency bugs me too.

After a lot of soul-searching over the past year or so I don't have a huge problem w/ ternaries, but something a little more terse w/o the requirement to handle the else case would really be nice.

barneycarroll commented 8 years ago

@lhorie it took me a while to get what you're saying :)

it follows that true should also yield a zero length text node

Agreed. Handlebars does if and unless. We should allow the same (except terser, and using Javascript). Besides, for people who don't find these logical expressions intuitive, it makes a lot of sense to simply say that Booleans aren't valid virtual DOM entities and will resolve to empty text nodes — authors can use this simple rule as they see fit to abstract whatever view patterns make sense to them.

This might help elucidate the issue:

m( Header ),
authenticated && m( AdminBar ),
m( Content )
m( '.exclusive-media-item',
  m( TeaserCard, item ),
  authenticated ||
  m( 'a', {
    href : '/login'
  }, 
    'Log in to read exclusive content'
  )
)

Digression:

foo || bar is far more intuitive than foo && bar (since "or" has a connotation of "either foo or bar, by process of elimination", but "and" has no such connotation)

This is the part that threw me off the scent: that doesn't ring true (haha) to me. A series of AND-delimited operands will return the last operand up until either termination, or an atomic operation evaluates to false. You might argue that that's not obvious to you and the explanation requires a deep understanding of low level language behaviour, but it's honestly totally intuitive to me (although I did have to think hard to formulate the technical description!).

if is a fundamental part of basic logic in any introduction to Javascript: unless isn't. What I'm getting at is the ability to use either of those confidently in Mithril views.


tl;dr

Let's allow the equivalent of if and unless in Mithril views via && and || respectively, by making boolean values in virtual DOM notation output empty text nodes.

syaiful6 commented 8 years ago

hmm, maybe just write helper that print nothing? here some equivalent for if and unless that print your vdom if true, end empty if false. My implementation is stupid, though. Take a look here, it use church boolean encoding: https://jsfiddle.net/syaiful6/yptL16e8/

syaiful6 commented 8 years ago

i am not sure if we should print nothing for boolean true or false. Use simple function for that task is prefered..hehe

dead-claudia commented 8 years ago

@syaiful6 The justification is things like this:

m(".page", [
  ctrl.admin && m(AdminButtons),
  ctrl.status === "unverified" || m(ProfileSelector),
  // etc...
])

Basically, allowing a shorthand for very simple and common cases. The alternative is this, which is admittedly a little inconvenient:

m(".page", [
  ctrl.admin ? m(AdminButtons) : null,
  ctrl.status === "unverified" ? null : m(ProfileSelector),
  // etc...
])
lhorie commented 8 years ago

Tagging w/ rewrite, since this is now implemented in v0.2.x stream

barneycarroll commented 8 years ago

:D

sebastiansandqvist commented 8 years ago

What do you think of coercing NaN also to a zero length text node?

pygy commented 8 years ago

@sebastiansandqvist in what scenario would it be useful?

sebastiansandqvist commented 8 years ago

@pygy I view it more as a consistency issue than anything else. It's the only falsy value that gets displayed (besides 0 which obviously should be displayed).

The case I encountered is having an input[type=number] whose value determines another component's text content by some equation. While there absolutely is good value in explicitly handling the case where the output is NaN, if it ever does occur I can't think of a single case where you'd want it in the ui.

dead-claudia commented 8 years ago

@sebastiansandqvist Could you give an example? I'm not entirely convinced it's common enough. And if necessary, you could always just do Number.isNaN(value) || m(Component).

sebastiansandqvist commented 8 years ago

@isiahmeadows On second thought I think it's better handled at the application level

orbitbot commented 8 years ago

This has already landed in next with #1103 , does the rewrite have further considerations or should it be a straightforward port?

pygy commented 8 years ago

Currently, the rewrite render engine skips null nodes, where the stable branch creates placeholders "" text nodes. This means that alternating between non-null and null vnodes shifts the next siblings back and forth, and is likely to cause a re-creation of the DOM nodes...

@lhorie wouldn't it be better to use placeholders as well? I suppose it would require patching both Vnode.normalize and render/render.js.

barneycarroll commented 7 years ago

I'd forgotten about this and thought I'd pop in and try to patch the old behaviour into new.

I was surprised to find there are all sorts of hyperscript tests relating to true and false which seem to explicitly invalidate the behaviour I describe in the OP. I don't understand why these are here. My instinct was to rip them out.

The special treatment of null - which as @pygy says is a breaking change - is baffling to me.

It seems that the behaviour I advocated for which got merged into v0.2.x has diverged dramatically in v1 but this seems more to do with oversight than intention - from a git blame it seems all of these tests and the null behaviour came in on the initial commit so the code and reasoning presumably pre-dates this issue.


@pygy I don't like the idea of sentinel values. { subtree : 'retain' } fulfilled such a function and was removed. Despite your suggested API being cleaner, it's an exotic concept which increases API and adds cognitive load for what is essentially an extremely simple feature with (what I think is) a much more intuitive and less magical current working method.


So: would anybody have any objection if I ripped out the tests that check against the logic in the OP, and merged the functionality into rewrite?

pygy commented 7 years ago

@pygy I don't like the idea of sentinel values. [snip]

@barneycarroll did you mean to ping me here? I don't remember suggesting API changes related to this... By "placeholders", I meant empty text nodes.

barneycarroll commented 7 years ago

I completely misunderstood — thought you were suggesting something like m.NOT_A_NODE to replace false.

So you're saying we should reintroduce placeholder nodes — because it ends up being more logical during diff?

pygy commented 7 years ago

Yes!

pygy commented 7 years ago

I was actually wrong about the collapsed nulls, sorry for the noise.

barneycarroll commented 7 years ago

I've come to the realisation that fragments make placeholder nodes redundant, while also making logical expressions easier to parse in hyperscript code, because it introduces delimiters.

One of the problems with simple logical operators is that the general assumption when visually parsing a chunk of hyperscript is that you can read the code sequentially and assume that each discrete reference will be interpolated as a vnode, and operators violate that assumption - you encounter the operator and have to re-assess the significance of the preceding operand. This is borne out by recent efforts in JS style guides to formalise strict whitespace rules concerning ternary operators. Fragments offer a nice delimiter for this purpose, which while strictly speaking exotic, I think makes things easier to parse.

The following is a strawman PoC that shows how all these features work together - don't judge it as a whole, because it isn't logically sound or rational in its focus & granularity - but take account of how each vnode & logical expression is parseable in context.

var Post = {
  view : vnode =>
    m(".wrapper",
      [
        vnode.state.loggedin
      ? m(ProfileWidget)
      : m("a", {
          href   :"/login",
          oninit : m.route
        }, "Log in")
      ],

      [
         vnode.state.writePermissions
      && m("p.status", "You can edit this document")
      ],

      m("h1", "Hello"),
      m("p", {
        contenteditable : vnode.state.editing 
      }, 
        m("span", "foo"),
        m("span", "bar")
      ),

      [
         vnode.state.writePermissions
      && [
           vnode.state.editing
         ? [
             m(CancelButton),
             m(SaveButton)
           ]
         : m(EditButton)
         ]
      ]
    )
}

So we now have a nice visual outline which IMO makes declarative virtual DOM easier to understand as a dynamic structure. Fragments act as delimiters. This means they can contain conditional nodes or lists of nodes. You can tell which kind at a glance via alignment (people who believe in comma-first lists will benefit from a stricter consistency). This bears out to the rendering logic, where eg the user logging out will not result in a dud placeholder DOM node being created to replace the write permissions status indicator, but neither will the diff engine get confused about the location of the subsequent h1 when that condition is toggled.

@pygy what do you think?

leeoniya commented 7 years ago

@barneycarroll

I think using parentheses rather than fragments for this purpose is both more logical and avoids extra logic-only vnode layers plus unnecessary array allocation in the template. It doesnt eliminate the need to coerce false but does nicely flatten things without incurring vtree overhead.

barneycarroll commented 7 years ago

@lhorie as discussed I'm removing the tests which validate Javascript's ability to coerce types when this is simply mundane native Javascript engine behaviour - with the exception of those where Mithril does perform special logic of its own - ie ensuring that empty strings are respected and zeros are coerced - because these represent edge cases to the falsey behaviour we're discussing.

barneycarroll commented 7 years ago

@leeoniya you're absolutely right - that's a much more general solution to the readability issue. Less exotic conventions, unambiguous localised logic rather than deferred processing. For the sake of comparison:

var Post = {
  view : vnode =>
    m(".wrapper",
      (
        vnode.state.loggedin
      ? m(ProfileWidget)
      : m("a", {
          href   :"/login",
          oninit : m.route
        }, "Log in")
      ),

      (
        vnode.state.writePermissions
      && m("p.status", "You can edit this document")
      ),

      m("h1", "Hello"),
      m("p", {
        contenteditable : vnode.state.editing 
      }, 
        m("span", "foo"),
        m("span", "bar")
      ),

      (
         vnode.state.writePermissions
      && (
           vnode.state.editing
         ? m(EditButton)
         : [
             m(CancelButton),
             m(SaveButton)
           ]
         )
      )
    )
}

That is so much nicer, but in practice it raises bugs. I'll split the render / diff concerns off to a new issue and keep this thread about false interpolation.

leeoniya commented 7 years ago

false handling could also live in the template with as few as 3 extra chars (per logic block):

      (
         vnode.state.writePermissions
      && (
           vnode.state.editing
         ? m(EditButton)
         : [
             m(CancelButton),
             m(SaveButton)
           ]
         )
      )
    ) || null
barneycarroll commented 7 years ago

@leeoniya IMO that's unnecessary and confusing. x && y || z is really confusing compared to x ? y : z. An expression consisting of sequential logical operations is extremely smelly IMO, and liable to totally turn off people who might otherwise be amenable to coming round from the idea that you need special 'helpers' for bog standard logical operations.

How is the code above more desirable for having || null sneakily suffixed at the end? Wouldn't it just be better off if it was semantically equivalent without?

(BTW, still don't understand the rationale for explicit handling of null at all FWIW)

leeoniya commented 7 years ago

How is the code above more desirable for having || null sneakily suffixed at the end? Wouldn't it just be better off if it was semantically equivalent without?

(BTW, still don't understand the rationale for explicit handling of null at all FWIW)

This may not apply to Mithril. Perhaps null-removal is particular to domvm; it's the only value that gets spliced out from templates by the preprocessor. true, false, NaN, etc. don't get special treatment. it may make sense for me to splice out false as well to accommodate this template format but i'm hesitant cause 2 special cases is a whole lot more than 1.

barneycarroll commented 7 years ago

@leeoniya well AFAIC null is a notoriously exotic value in JS. It's confusing and usually signifies an error of some kind, so encouraging authors to supply it as part of your API isn't a priori a good idea IMO. Why not simply replace the null check with a false check? Makes stuff easier in author land without having to introduce extra conditions in your code.

leeoniya commented 7 years ago

a big part of it is that internally foo == null will test for either null or undefined in a single expression (perf, terse). relying on a loose null test instead of a strict false test can be useful in some cases of sloppy template authoring. it also has the benefit of being a distinct/reserved type [though technically it isnt] alongside undefined that semantically indicates "absence of explicit value" without introducing a cognitive two-way split in boolean interpretation.

barneycarroll commented 7 years ago

I agree that extending the operating logic around falseyness is ugly. We should cut down on that. It's awkward that a range of falsey values should have specific semantics while others don't.

But while it may be nice in the abstract that undefined == null, as a feature this presumes that it's of benefit to the user to be able to explicitly supply null or have an interpolated model's value happen to be undefined and end up with the same outcome. I don't think that holds water. The first scenario is a somewhat arbitrary (and in the case of Mithril, undocumented) hyperscript feature. The second is a silent application error IMO.

Mithril is much more clear in the DOM it produces and easier to debug as an app framework - so you can take some of the sting out of this anecdote - but when working with Ember I found it absolutely infuriating that unexpectedly undefined model values resulted in no DOM. Particularly when you are interpolating a series of model values one after the other in a string this is extremely confusing, and very difficult to debug if you're not familiar with the rendering engine and data flow.

Ultimately you have to ask yourself:

  1. Are people really designing models where they actually expect undefined to silently collapse to nothing when they try to output it to the UI? Should they?
  2. In the case where you expect a model to generate a string or a vnode of some form but it interpolates undefined, and this is an unexpected application error, is it better to gloss over it or stringify "undefined"? Personally, I'd prefer the latter because I can get immediate feedback and quickly identify the problem - the former strikes me as unaccountably opinionated about fault tolerance (some of your model is broken but eh, we'll do the other bits anyway).

In contrast, false being a unique special case makes a lot of sense. Aside from the huge convenience of condition && outcome, it matches the behaviour of boolean attrs, where false indicates the attribute node should be removed.

leeoniya commented 7 years ago

Those are good points. Will have to give this a shot, maybe even a perf boost is in there.

dead-claudia commented 7 years ago

@barneycarroll @leeoniya

Nodes that are == null are currently ignored in the rendered tree in both 0.2 and 1.x. Just thought I'd clear that up.

Also, relying on !node is generally slower than node == null, simply because here's what each does:

In practice, with inline caches, the latter can be optimized to something close to as fast as the former if the types are consistent, but in this particular scenario, it won't happen in practice because the nodes' types are usually very highly polymorphic.

barneycarroll commented 7 years ago

@isiahmeadows agreed — testing for falsey vnodes would be both expensive and undesirable.


EDIT: Worth pointing out in this context that my proposal — testing for strict false — involves less overhead than the current implementation.