Skip to content
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

Allow children to be specified as a single function returning a vnode tree #2050

Closed
dead-claudia opened this issue Dec 6, 2017 · 12 comments · Fixed by #2155
Closed

Allow children to be specified as a single function returning a vnode tree #2050

dead-claudia opened this issue Dec 6, 2017 · 12 comments · Fixed by #2155
Labels
Type: Enhancement For any feature request or suggestion that isn't a bug fix

Comments

@dead-claudia
Copy link
Member

dead-claudia commented Dec 6, 2017

Expected Behavior

This came up on Gitter in a discussion there.

Basically, I'd like to see this to happen:

  • If a child is a function, it should not be normalized.
  • Only one child is allowed in this special case, as it replaces .children altogether.
  • DOM vnodes do not support children as functions

Current Behavior

Currently, functions are just toString()'d, which is hardly helpful ever.

Possible Solution

A few small changes to Vnode.normalize, Vnode.normalizeChildren, and m.

Steps to Reproduce (for bugs)

Context

This is to support reuse of child vnodes without actually requiring identity (which we all know is unsafe). It also allows children to be parameterized in cases like with React's react-motion, and it'd make Mithril a little more useful for cases where you have to deal with animations/etc. (and other changing data independent of the global model state).

A couple other examples where this could've improved or otherwise affected API design:

Also, if you've ever used a pattern like this, this feature requests aims to reify and partially replace it:

// Now
var List = {
    view({attrs}) {
        items = transform(attrs.items)
        return m("div", [attrs.view ? attrs.view(items) : items])
    }
}

m(List, {
    items: [...],
    view(items) {
        return items.map(item => m("div", ...))
    },
})

// Proposed
var List = {
    view({attrs, children = items => items + ""}) {
        items = transform(vnode.attrs.items)
        return m("div", [children(items)])
    }
}

m(List, {items: [...]}, items => {
    return items.map(item => m("div", ...))
})

Your Environment

  • Version used:
  • Browser Name and version:
  • Operating System and version (desktop or mobile):
  • Link to your project:
@dead-claudia dead-claudia added the Type: Enhancement For any feature request or suggestion that isn't a bug fix label Dec 6, 2017
@dead-claudia
Copy link
Member Author

Yes, if you follow React at all, this sounds a lot like their new "render props" fad. But it's not something Mithril users haven't been doing for a while through attributes instead. I just wanted to make a common pattern easier.

@pygy
Copy link
Member

pygy commented Dec 27, 2017

@isiahmeadows This would be a -0.1 from me, in that it doesn't add any functionality. children in components are mostly there to support JSX, overloading them doesn't seem that useful and adds complexity (more things to document, another thing to know about the framework when discovering a new codebase)...

This seems as clear as your proposal:

var List = {
    view({attrs: {items, render = items => items + ""}}) {
        return m("div", render(transform(items)))
    }
}

m(List, {
  items: [...]
  render: items => items.map(item => m("div", ...))
})

@dead-claudia
Copy link
Member Author

dead-claudia commented Dec 27, 2017

Okay. That's basically what I did with my SelfSufficient helper when I redid it. It's just awkward when it's a fairly frequently used utility. I'm considering moving it back behind a helper method, though, to hide some of the nastiness. (I'll have better timings control, too.)

@cavemansspa
Copy link
Contributor

this is a somewhat contrived example, but is there a recommended decorator pattern to augment a component that has children?

the use case is to let a user defined component be left intact, but let's say we want to provide a way decorate a component with functionality via a 3rd party util.

@dead-claudia
Copy link
Member Author

dead-claudia commented Dec 28, 2017

@cavemansspa There's a view/render attribute, there's a component attribute (when you need to render a component with your own attributes, not just simply a tree), and there's higher order components (what you used). Note that higher order components don't compose very well without issues (something the React community learned the hard way).

@pygy
Copy link
Member

pygy commented Mar 17, 2018

Re-opening this following #2106. cc/ @barneycarroll.

@barneycarroll
Copy link
Member

I missed this when it was first raised and wished I hadn't. Since then I've put a lot of work in exploring 'function-children' components. I last left #2106 with the thought that this was an acceptable contrivance for authors of a very specific kind of component.

But the more I think about it, the more I get @pygy's uneasy change of mood, and I'm more inclined to frame this as a case of the current vnode normalisation implementation being dangerously eager (as in WTF) on principle rather than the proposed use case necessarily deserving first class treatment (why shouldn't it?).

I've been racking my brain over a few avenues for resolving this ambiguity, and I think a couple actions should be taken:

  1. Remove .toString() & .valueOf() from stream. I remember asking Leo to implement this feature, thinking "wouldn't it be nice to avoid typing those parentheses". In hindsight that was an example of very short-sighted enthusiasm for a clever hack. I've never used it and I don't think anybody has (or ever should!) document it.
  2. Vnode normalisation should hold off on normalising the children of components. The children passed in to a component are fundamentally different to the children of any other kind of node in that the latter need to be interpolated immediately to get a recursive static representation of the virtual node in question: there is no other opportunity. In contrast, a component's children are a very different matter in that they may never be rendered. It's assumed they will be, but if when where and how is a matter for the component's view function. The decision to render whatever 'children' were passed in is deferred, and the contents of the view need to be normalised anyway — therefore, let's create an exception in vnode normalisation for the case of components that leaves children untouched.

I think that would also go some way to addressing Isiah's previously stated gripes with the contradictions of the documentation's advice on how to treat children. All in all we might all have been a bit guilty of condensingly paternalistism. The kids are alright.

@pygy
Copy link
Member

pygy commented Apr 10, 2018

I need to think 2) through, but at a glance, it looks like a good plan (for v2).

How is 1) related to this?

@barneycarroll
Copy link
Member

Sorry @pygy I missed this response. I am about to file an issue regarding the specifics of point 1, which has since been revealed to be a bug in its own right.

It relates to this issue inasmuch as Mithril's vnode type is very tolerant of what input can constitute a vnode in part because it leaves the door open to complex types with toString or valueOf to be treated as text nodes without coercing them to text nodes in the intermediary representation. It is essentially a very generous fall-through which spreads reasoning about what the entity being normalized actually is through various different steps, none of which really take responsibility.

@dead-claudia
Copy link
Member Author

In light of some recent discussion on #2275, I'm resurrecting this.

@barneycarroll
Copy link
Member

@isiahmeadows I think current v2 behaviour is good enough, personally. Check it out: we can event do the neat-o React array destructuring thing!

const ViewComponent = {
  view: ({children: [view]}) =>
    view('Hello!'),
}

@dead-claudia
Copy link
Member Author

Yeah, since that works, I'm closing it for now. I've got ideas in #2279/#2295 (comment), but I'm closing this in favor of what turns of that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement For any feature request or suggestion that isn't a bug fix
Projects
Status: Completed/Declined
Development

Successfully merging a pull request may close this issue.

4 participants