Simplify component constructor to function that returns either a view function or a vnode tree

[dead-claudia]

Mithril version:

Platform and OS:

Project:

Is this something you're interested in implementing yourself?

Description

Depends on #2688 and #2689 - you can see the first because of the two-argument view. It's easier to explain relative to this.

Replace these idioms:

const Comp = {
    view(vnode, old) {
        return tree
    }
}

function Comp(initial) {
    return {
        view(vnode, old) {
            return tree
        }
    }
}

With these:

function Comp(attrs, old) {
    return tree
}

function Comp(initial) {
    return (attrs, old) => {
        return tree
    }
}

As the rest of vnode.* properties are subsumed with other vnodes, I'd just provide the attrs themselves instead of full vnodes. (Vnode children would be moved to attrs.children to align with virtually every other framework out there - I don't see the benefit in us remaining special here.)

Why

1. It's simpler to write.
2. It's simpler to implement.
3. It's lower overhead to invoke.
4. Not much of a reason, but TS compatibility would be increased with this. If TS isn't, it'd be a much easier ask to get them to correct the incompatibility.

Possible Implementation

1. In createNode, change state initialization to just this:

var instance = (0, vnode.tag)(vnode.attrs)
if (typeof instance === "function") {
    vnode.state = instance
    vnode.instance = instance(vnode.attrs)
} else {
    vnode.state = vnode.tag
    vnode.instance = instance
}
// render vnode.instance as usual

2. In updateNode, invoke (0, vnode.state)(vnode.attrs, old.attrs) instead of vnode.state.view(vnode, old).

Open Questions

• Should we also kill onInit from m.access(...) in Separate lifecycle methods from attributes and components #2689? I say no, because that would make it so users could still have that functionality outside components.

[gilbert]

As the author of mithril-cc, naturally I'm excited about this 😄

Regarding initial in the closure component form, I'm concerned the age-old stale attrs problem will rear its ugly head again.

For comparison, react hooks don't have this problem since the component function gets invoked every redraw. Not that it's a better solution by any means, as hooks create many more subtle problems when it comes to closures, but the point is react users never have to think about the current state of their props.

With mithril-cc I solved this problem by providing initial as a update stream rather than a plain object. That may not be the right solution for this proposal since mithril prefers to be unopinionated. But with that in mind, a getter function would equally solve the problem in a less opinionated way.

[dead-claudia]

@gilbert Might not be a bad idea, but that does come with some performance cost (V8 doesn't inline closure accesses properly for one, and you'd have to memoize it for non-closure components), and it's also a bit awkward IMHO.

We can use documentation to warn people of this gotcha, though, and it only applies for stateful components.

[gilbert]

Ah right, since both versions are functions, mithril doesn't know what version your component constructor is until after it runs once, which poses a problem.

[dead-claudia]

And also I'm not a fan of presenting two different ways of getting attributes for two different component types. Sounds ripe for confusion IMHO.

[gilbert]

The stale attrs problem is more confusing IMO, as it's much harder to document (not to mention its inelegant workarounds). But it seems there's not much mithril can do here, other than assuming function components will always return closures, which... isn't terrible, but it is more opinionated. I'll probably have to just stick with using mithril-cc.

Speaking of opinionated, would this change mean mithril is dropping support for class components?

[dead-claudia]

The stale attrs problem is more confusing IMO, as it's much harder to document (not to mention its inelegant workarounds). But it seems there's not much mithril can do here, other than assuming function components will always return closures, which... isn't terrible, but it is more opinionated. I'll probably have to just stick with using mithril-cc.

The problem is really that there's no nice alternative that doesn't have major pitfalls.

Option 1: my proposal

// Oops, shadowed
function Foo(attrs) {
  return () => m("div", {class: attrs.class})
}

Option 2: use a function reference (your suggestion in Gitter)

// Works, but isn't very optimizable by engines
function Foo(attrs) {
  return () => m("div", {class: attrs().class})
}

Option 3: use an instance property (what React did)

// Variant 1: class
// Property updated by the renderer before each update
m.Component = function (attrs) { this.attrs = attrs }

// Oops: `this` isn't the class instance
class Foo extends m.Component {
  view() { return m("button", {onclick() { Model.incrementItem(this.attrs.id) }}) }
}

// Variant 2: `this`-driven
// Oops: `this` isn't the class instance
function Foo() {
  return () => m("button", {onclick() { Model.incrementItem(this.attrs.id) }})
}

Option 4: use an object reference

// Property updated by the renderer before each update
// Gets awkward in a hurry, but if we shorten it to `i.attrs.class`, it might be
// workable.
function Foo(context) {
  return () => m("div", {class: context.attrs.class})
}

Option 5: use hooks

// Either it ends up rigid on diffing or bloats the bundle significantly, and it's
// not concise at all in practice. It's also prone to leaving you with stale
// closures.
function Counter() {
  const [count, setCount] = slot(0)
  return [
    m(".display", count),
    m("button", {onclick() { setCount(count + 1) }}, count)
  ]
}

Option 6: use streams

// This is actually very involved to wire up correctly and involves a lot of edge
// cases, just FYI. I've looked into this model already, and we'd have to rewrite
// our renderer for it.
function Foo(attrs$) {
  return attrs$.map(attrs => m("div", {class: attrs.class}))
}

Here's my analysis of each:

1. Brought up already, but there's a risk of forgetting to shadow attributes, especially if you don't use the initialAttrs idiom.
2. As noted, engines struggle to optimize that pattern. Unless something's changed in the past year or so, V8 can't inline that (I've verified it myself), and engines have to inline the closure call before they can even optimize property access. Additionally, we'd have to not only keep a getter closure for components (and store it accordingly), but allocate either a single-property object or a setter closure just to update it later. And that is a lot of memory to waste for a component.
3. Anything with this is a non-starter for me. Unless you're using TypeScript, it's a recipe for disaster, and even TypeScript users have little to gain by using classes.
4. I like this one, particularly if you shorten the idiomatic parameter name as I suggested there (to like i.attrs). You could also add other properties (I've looked into this space already):
   • i.isInitial for reasons you can probably infer.
   • i.request (for m.request), and i.meta = {window, document, type: "dom" | "static"} (the first two can be retrieved from doc.defaultView and elem.ownerDocument respectively), to enable consumers to not rely on globals to do XHRs and the like (useful with JSDOM) and to allow them to tell apart different renderer types (making the framework considerably more portable).
   • i.state and i.link(init) (I'm not sold on this without it), where i.link = init => i.isInitial ? (i.state = init()) : i.state, for setting up mutable state, instead of using a closure. (You could use if (i.isInitial) i.state = {...}, but a dedicated method is cleaner IMHO and offers better flexibility.)
   • We could put the previous attributes in an optional second parameter, retaining that convenience without needing to pollute the state object with another property.
5. This is a non-starter for me. I've explored it already, and have used React Hooks extensively enough to know both where it excels (modular state requires less setup boilerplate) and where it doesn't (anything involving conditionals and mutable state that doesn't fit in its pretty architecture). And of course, it comes with other pitfalls as I've noted in the example.
6. This is once again a non-starter for me. I've tried figuring out how to wire view streams already, and while it make for concise components, it's an utter beast to set up properly, and it's riddled with edge cases. I could see a modified model being at least somewhat useful for statically compiled frameworks down the vein of Svelte, but if we're talking a fully managed pure runtime component, this belongs in userland.

Of all these, I'm leaning towards option 4. @gilbert @barneycarroll What do you think?

Speaking of opinionated, would this change mean mithril is dropping support for class components?

Yes.

[StephanHoyer]

I would (again) suggest another version, which I now use for quite a while with a small helper (withState).

function withState(getView) {
  const comp = {
    oninit: ({ state, attrs }) => (state.viewFn = getView(...attrs.args)),
    view: ({ state, attrs }) => state.viewFn(...attrs.args),
  }
  return (...args) => m(comp, { args })
}

Than, I only use "functional" components (aka view-functions), that return trees. If I need state I create a closure that returns a view function just like your second example. I pack this up with my helper to mark is a component. The result is again a view function. This way it's transparent for the consumer, if the component is just a function or a component. This way you can easy switch from one to the other without changing the call-site.

const compView = withState((...initialArgs) => (...args) => tree)

// usage
compView(...args) // returns tree

I think this would also go great in conjunction with m.access and alike.

[gilbert]

Another con of Option 2 is the dilemma between only providing the attrs getter, or providing a plain object attrs in the returned view function but risking a similar shadowing problem like Option 1.

Sleeping a bit on Option 4, I'm starting to like it. Aside from the benefits you list, i could also inherit from an extendable prototype, allowing users and libraries to extend component behavior in interesting ways.

On the other hand, @StephanHoyer's example makes me wonder, does mithril need to handle state at all? Could it only worry about component identity and handoff state approaches to the user? Perhaps with a few "canonical" helpers.

[orbitbot]

On the other hand, @StephanHoyer's example makes me wonder, does mithril need to handle state at all? Could it only worry about component identity and handoff state approaches to the user? Perhaps with a few "canonical" helpers.

@gilbert

I'm a bit unsure of how to parse this, could you elaborate?

[orbitbot]

// Property updated by the renderer before each update
// Gets awkward in a hurry, but if we shorten it to i.attrs.class, it might be
// workable.

Code examples can use destructuring syntax by now, and context has a common abbreviation in ctx which is better than one character shorthands.

[dead-claudia]

// Property updated by the renderer before each update

// Gets awkward in a hurry, but if we shorten it to i.attrs.class, it might be

// workable.

Code examples can use destructuring syntax by now, and context has a common abbreviation in ctx which is better than one character shorthands.

@orbitbot True, and in practice I anticipate a lot of destructuring with it. This is more in reference to keeping the most recent value at all times.

[dead-claudia]

@StephanHoyer Okay, that makes me think an option 7 where we just use m.state(i => vnode) where i is as suggested above but without the vnode, and we could just provide m(Comp, attrs?, ...children) as sugar for m.link(Comp, Comp({...attrs, children})) for JSX users.

[gilbert]

Code examples can use destructuring syntax by now

You have to be careful where you destructure:

// Good
function Foo(ctx) {
  return () => {
    let { attrs } = ctx
    return m("div", {class: attrs.class})
  }
}

// Bad, attrs becomes stale
function Foo({ attrs }) {
  return () => m("div", {class: attrs.class})
}

I would add this as a con to 4.

[orbitbot]

Code examples can use destructuring syntax by now

You have to be careful where you destructure:

// Good
function Foo(ctx) {
  return () => {
    let { attrs } = ctx
    return m("div", {class: attrs.class})
  }
}

// Bad, attrs becomes stale
function Foo({ attrs }) {
  return () => m("div", {class: attrs.class})
}

I would add this as a con to 4.

Isn't this essentially the exact same thing as with the current syntax, apart from actually being able to access the correct attrs as they're the same object references?

• i.isInitial for reasons you can probably infer.
• i.request (for m.request), and i.meta = {window, document, type: "dom" | "static"} (the first two can be retrieved from doc.defaultView and elem.ownerDocument respectively), to enable consumers to not rely on globals to do XHRs and the like (useful with JSDOM) and to allow them to tell apart different renderer types (making the framework considerably more portable).

Are these things that should actually be provided by the framework to each component, as a prima vista it feels a bit odd to have these "global methods" re-referred in individual components? Also this could be achieved in userland?

It may well be that I'm not immediately recognising the opportunities this affordance would enable, though.

[dead-claudia]

@gilbert

Code examples can use destructuring syntax by now

You have to be careful where you destructure:

// Good
function Foo(ctx) {
  return () => {
    let { attrs } = ctx
    return m("div", {class: attrs.class})
  }
}

// Bad, attrs becomes stale
function Foo({ attrs }) {
  return () => m("div", {class: attrs.class})
}

I would add this as a con to 4.

If we go the context route, I'll also be pushing to add context.state and context.link as explained above, which will completely avoid the closure problem by...just not using closures to model state.

function Counter(i) {
  const state = i.link(() => ({count: i.attrs.initial ?? 0})
  return [
    m(".display", state.count),
    m("button", {onclick() { state.count++ }}, count)
  ]
}

Stale attributes is still possible (JS doesn't have a way to access by reference except via object properties and closures), but it'll be much less counterintuitive why.

[dead-claudia]

@orbitbot

• i.isInitial for reasons you can probably infer.
• i.request (for m.request), and i.meta = {window, document, type: "dom" | "static"} (the first two can be retrieved from doc.defaultView and elem.ownerDocument respectively), to enable consumers to not rely on globals to do XHRs and the like (useful with JSDOM) and to allow them to tell apart different renderer types (making the framework considerably more portable).

Are these things that should actually be provided by the framework to each component, as a prima vista it feels a bit odd to have these "global methods" re-referred in individual components? Also this could be achieved in userland?

It may well be that I'm not immediately recognising the opportunities this affordance would enable, though.

We could just have m.request take a window option (it's to get relevant constructors). But there's benefits to injecting these globals in components:

1. You probably want to skip your widget initialization when rendering to a static string, and if that has to be done before rendering the view, this makes it far easier to branch off of.
2. If your app architecture supports it, you can do all your tests without the error-prone boilerplate of injecting globals. (I actually found issues within Mithril's tests in Move to Rollup internally, split tests and source, and simplify a *bunch* of things #2677 because we were accidentally relying on globals - imagine if we forgot to reset one of them. 😬)
3. i.isInitial is there in part for convenience and in part to assist my suggested i.link (so it's not branching based on undefined). If we decide to treat undefined as a special value, I'm open to nixing this as m.access already sends that to its callback.
4. It provides much more clearly defined points for people to write their own custom renderers based on Mithril's own, and it'd in theory let us split the actual renderer from the rest of the framework. While this on the surface doesn't seem like much, I'd like to open the door for non-core renderers, especially ones that aren't static like mithril-node-render.

[gilbert]

If we go the context route, I'll also be pushing to add context.state and context.link as explained above, which will completely avoid the closure problem by...just not using closures to model state.

I dislike this style of state as it quickly gets clumsy if you need one state property to reference another. For example:

function Counter(ctx) {
  const state = ctx.link(() => {
    const $count = m.stream(ctx.attrs.initial ?? 0)
    const $changeCount = m.stream(-1)
    $count.map(() => $changeCount($changeCount() + 1))
    return { $count, $changeCount } // :(
  })
  return [
    m(".display", state.$count()),
    m("button", {onclick() { state.$count(state.$count() + 1) }}, state.$count())
  ]
}

And that's just with two properties. This object-return repetition makes it slightly worse than a class syntax, and without the convenience of instance methods:

Class syntax equivalent

class Counter {
  constructor({ attrs }) {
    this.$count = m.stream(ctx.attrs.initial ?? 0)
    this.$changeCount = m.stream(-1)
    this.$count.map(() => this.$changeCount(this.$changeCount() + 1))
  }
  view = () => [
    m(".display", this.$count()),
    m("button", {onclick() { this.$count(this.$count() + 1) }}, this.$count())
  ]
}

Of course, the closure component style is much nicer. Maybe we just need to emphasize to never destructure attrs. It's a simple rule to remember, even if its stroke is broader than strictly necessary.

function Counter(ctx) {
  const $count = m.stream(ctx.attrs.initial ?? 0)
  const $changeCount = m.stream(-1)
  $count.map(() => $changeCount($changeCount() + 1))

  return () => [
    m(".display", $count()),
    m("button", {onclick() { $count($count() + 1) }}, $count())
  ]
}

I'd like to open the door for non-core renderers

I was thinking the same thing. Let's give some competition to react native :)

I don't want to derail the conversation too much, but would a ctx.redraw() be theoretically possible for granular redraws?

[dead-claudia]

Edit: @gilbert (forgot to tag you)

Of course, the closure component style is much nicer. Maybe we just need to emphasize to never destructure attrs. It's a simple rule to remember, even if its stroke is broader than strictly necessary.

Yeah, I really don't like it (having a context object plus a closure), but it seems to be the best way to go. Maybe the following types are best:

function Comp(ctx) {
  return (prevAttrs?) => vnode
}

function Comp(ctx, prevAttrs?) {
  return vnode
}

(Of course, prevAttrs is as noted in #2688.)

There would of course be no public ctx.state. I will note that only for the closure version is it necessary to never destructure.

I was thinking the same thing. Let's give some competition to react native :)

😎

I don't want to derail the conversation too much, but would a ctx.redraw() be theoretically possible for granular redraws?

If the renderer in question supports it, then sure. Not that I'd bake it into our size-optimized core renderer, though - I'd leave that mess to someone else to implement. But either way, it'd necessarily need to be on the context if you want to retain portability (and it'd give us a chance to banish global multi-root redraws while still allowing multiple roots - I really don't like our current system as it redraws way too many things).

[orbitbot]

Chewing on the latest a bit, I don't really understand the value proposition of i.state and i.link, as it kind of mainly seems like a larger surface API to achieve what is already available...

We could just have m.request take a window option (it's to get relevant constructors).

👍 Feels like a well-defined enough idea that it can be worked on separately. Wonder if this by itself would roughly be enough for using one of the XHR-aping node packages to work... Wouldn't be opposed to kicking the tires on this myself.

1. You probably want to skip your widget initialization when rendering to a static string, and if that has to be done before rendering the view, this makes it far easier to branch off of.

👍

1. If your app architecture supports it, you can do all your tests without the error-prone boilerplate of injecting globals. (I actually found issues within Mithril's tests in Move to Rollup internally, split tests and source, and simplify a *bunch* of things #2677 because we were accidentally relying on globals - imagine if we forgot to reset one of them. 😬)
2. i.isInitial is there in part for convenience and in part to assist my suggested i.link (so it's not branching based on undefined). If we decide to treat undefined as a special value, I'm open to nixing this as m.access already sends that to its callback.
3. It provides much more clearly defined points for people to write their own custom renderers based on Mithril's own, and it'd in theory let us split the actual renderer from the rest of the framework. While this on the surface doesn't seem like much, I'd like to open the door for non-core renderers, especially ones that aren't static like mithril-node-render.

Fair enough. Not sure I have a grasp of what the reserved words of the ctx parameter might be in this case, if/when it's different to the current vnode of 1.x and 2x, but the thought came up that perhaps these "secondary"/"advanced" (?) use-cases would be hidden in a separate namespace in the parameter...

Which might become something like this(?) 🤔

function Comp(ctx, prevAttrs?) {
  return m('h1.arbitrary',
    m.access((dom, init) => ctx.mithril.request('/offplanet', ...), 
    'actualheadingtext')
}

[dead-claudia]

@orbitbot

Fair enough. Not sure I have a grasp of what the reserved words of the ctx parameter might be in this case, if/when it's different to the current vnode of 1.x and 2x, but the thought came up that perhaps these "secondary"/"advanced" (?) use-cases would be hidden in a separate namespace in the parameter...

Which might become something like this(?) 🤔

Yeah, I'll stop including that prevAttrs for now to reduce confusion. (I'm already second guessing that particular design, and I'll be updating #2688 accordingly.)

But based on my suggestion with m.request, it'd look closer to this:

function Comp(ctx) {
  m.request('/offplanet', {window: ctx.window, ...}).then(value => ctx.redraw())
  return () => m('h1.arbitrary', 'actualheadingtext')
}

Part of what drew me into m.request being ctx.meta.request or similar was that you could use most of the same underlying APIs on both native and DOM without needing to change anything about your code, even without mocking the DOM or anything like that. But I'm hesitant to go that far, especially when it's non-trivial to do similar to the router (even though at a high level it naturally follows).

[dead-claudia]

@orbitbot Caught this in Gitter:

const Comp = () => {
  const poll = (value, { mithril }) => value === '123' && mithril.request('offplanet', ...)

  return (ctx) => m('input', m.access(({ value }) => poll(value, ctx)), 'somestring')
}

Makes me wonder if it'd be better to structure the API like this:

function Comp(ctx) {
  return ctx => vnode
}

function Comp(ctx) {
  return vnode
}

That'd simplify dispatch a little while still providing all the same benefits. And as ctx holds the same identity in both places, shadowing isn't actually an issue.

---

I do have a strong bias towards components as functions as functions naturally have names and this aids tremendously in debuggability. (I've already in the wild have had issues debugging object components, and have started to use closure components even for stateless components just to recover some of that, despite it being more boilerplate.) And if someone in the future writes dev tools integration, this would make their life easier, too.

[gilbert]

Caught this in Gitter:

Providing ctx to both is a great solution IMO. And to add and comment on the conversation, @barneycarroll writes:

Personally I’d avoid lifecycle entirely
I think of effects as purely DOM related
Stateful integration stuff belongs in the component closure IMO

I agree with this. For the record, I've had to use oncreate as a way to avoid making API calls during SSR with mithril-node-render. If ctx is extendable by libraries, mithril-node-render could add something like a ctx.isServer property to help with this.

[StephanHoyer]

Just FYI

A problem I ran into with this style is that in this case ctx is shadowed by the inner function, which raises a warning in eslint (if no-shadow on). It's probably even helpful here to become aware of potentially stale references.

I tend to use object destruction and use different properties for init and view.

[dead-claudia]

@StephanHoyer

A problem I ran into with this style is that in this case ctx is shadowed by the inner function, which raises a warning in eslint (if no-shadow on). It's probably even helpful here to become aware of potentially stale references.

As I noted before, they're the same reference, so there really isn't a semantic difference to beware of.

Of course, ESLint may warn, and in that case, the ideal fix is to just pick a spot, one or the other.

[panoply]

Making noise on this one for discussion of v3 #2754.