Replace key: with m.link(...) and m.track(...)

[dead-claudia]

Mithril version:

Platform and OS:

Project:

Is this something you're interested in implementing yourself?

Description

Replace the key: attribute with two helpers:

• m.link(key, ...children) to replace the single-child keyed fragment idiom
• m.track([{key, value}, ...]) to replace collections of keyed elements

The tag of m.link(...) would be "$", and the tag of m.track(...) would be "@".

Related: #2690, #2689, #2689

Why

Couple reasons:

1. The single-child keyed fragment idiom is not obvious for newcomers and even intermediate developers in my experience. I already had to teach my workplace about the idiom in the context of React, and Mithril is no different, and it's also historically been one of the most common sources of confusion and Gitter questions. I'd like to separate that out to hopefully provide a better and more intuitive path for users.
2. By making keyed lists their own separate vnode type, I can optimize their representation to reduce memory churn. I can also avoid the need for code like this and this, further driving down the overhead of constructing vnode trees because I can just do return input.map(Vnode.normalize) there and it magically works.

Possible Implementation

This is of course the complicated part and would require a lot of code movement. TL;DR:

• This dispatch boilerplate goes away
• vnode.key goes away, and instead I'd use vnode.state to hold the key for m.link(...) and key list for m.track(...).
  • By side effect, this would no longer need to access properties, and would end up significantly faster because of it.
  • By pre-serializing in m.track(...) to separate key and child lists, this would mean we can still keep vnode.children set to a raw array and not have to specially handle it in the removal logic.
• This chunk of code would be moved to a function invoked by this switch statement for the vnode tag "@".
• A new code path would exist for vnode.tag === "$" in createNode and updateNode as applicable, largely delegating to what's used by fragments.
• Most everything else .key-related could just die.

Open Questions

• The structure of m.track - is that okay, or should it be changed to an array or something else? An object will slightly grow the size of most multiline keyed fragments in lines of code, but not significantly in minified size, and an array would be slightly smaller but less immediately clear just from reading it.
• Whether this is even a good idea. I like it as it's addressing actual issues I've seen over the years, but of course this is just my personal anectdata.
• Edit: Names - I wish I could find something better. .bind already exists - it's Function.prototype.bind, and I really don't want to go down that rabbit hole (you'll inevitably run people into really arcane edge cases fast, and even Babel historically relied on it for some things). I've considered m.collect before going with m.track, but yeah, I'm not fond of either to be quite honest.

[StephanHoyer]

I don't really get this. Can you do a little example on how you would do a keyed list?

[CreaturesInUnitards]

I'm good with this in theory, but I'm confused about the semantic choice of m.link

[barneycarroll]

I agree with Scotty, link is no good IMO. track, though, is very good.

The crucial problem with key as currently implemented by most people is it’s a pattern featuring an API that requires the surrounding structure to conform to a certain criteria, and intent isn’t readily apparent from a buggy use case. The problem with the single-item-keyed-list is a consumer-land hack making use of emergent behaviour.

So I reason that since we have two distinct enough signatures we can still capture intent and produce pertinent warnings in cases where the input isn’t conformant — track adequately describes both use cases:

m(Header),

[showModal && m(Modal, {key: 'modal'})],

m(PageContent)

// vs

m(Header),

m.track('modal', showModal && m(Modal),

m(PageContent)

[dead-claudia]

Edit: fix a name, add contrast

@CreaturesInUnitards @barneycarroll @StephanHoyer Names are easily bikeshed. But the idea is that you could do this:

// Utility component
function Async(initialProps) {
  const ctrl = new AbortController()
  let state = "pending"
  let value
  new Promise(resolve => resolve(initialProps.fetch(ctrl.signal))).then(
    v => { state = "ready"; value = v },
    e => { state = "error"; value = e }
  )

  return props => [
    m.release(() => ctrl.abort()),
    // Don't diff `pending` and `ready`/`error`, just replace entirely.
    m.link(state,
      state === "pending" ? props.pending()
      : state === "ready" ? props.ready(value)
      : props.error(value)
    ),
  ]
}

// This automatically re-fetches every time you change stores, and always keeps the product
// availability count up to date when you change the filter, all with zero resource leaks.
function ProductTable({storeId}) {
  let filter = "all"
  return () => m(".item-table",
    m(".item-header", props.header),
    m(".item-filter", "Item type: ", m("select",
      m("option", {value: "all", selected: filter === "all"}, "All"),
      m("option", {value: "shirts", selected: filter === "shirts"}, "Shirts"),
      m("option", {value: "shorts", selected: filter === "shorts"}, "Shorts"),
      m("option", {value: "skirts", selected: filter === "skirts"}, "Skirts"),
      m("option", {value: "jackets", selected: filter === "jackets"}, "Jackets")
    )),

    m.link(id, m(Async, {
      fetch: signal => m.request("/api/:storeId/products", {params: {storeId}, signal}),
      pending: () => m(LoadingSpinner, {variant: "large"}),
      error: e => m(ErrorDisplay, {error: e}),
      ready: ({products}) => m("table",
        m("tr",
          m("th", "Name"),
          m("th", "Description"),
          m("th", "Available")
        ),
        m.track(
          products
          .filter(product => filter === "all" || product.type === filter)
          .map(product => ({
            key: item.id,
            value: m("tr",
              m("td", m(m.route.Link, {href: product.id}, product.name),
              m("td", product.shortDesc),
              m("td", m(Async, {
                fetch: signal => m.request("/api/:storeId/products/:id/inventory", {
                  params: {storeId, id: item.id},
                  signal,
                }),
                pending: () => m(LoadingSpinner, {variant: "small"}),
                error: e => m(ErrorDisplay, {error: e}),
                ready: inventory => inventory.available,
              }))),
            ),
          })
        ),
      )
    }))
  )
}

With key attributes, it's a little cloudier and you have to look a little harder to see what's going on. It doesn't pop as well.

// Utility component
function Async(initialProps) {
  const ctrl = new AbortController()
  let state = "pending"
  let value
  new Promise(resolve => resolve(initialProps.fetch(ctrl.signal))).then(
    v => { state = "ready"; value = v },
    e => { state = "error"; value = e }
  )

  return props => [
    m.release(() => ctrl.abort()),
    // Don't diff `pending` and `ready`/`error`, just replace entirely.
    [m.fragment({key: state},
      state === "pending" ? props.pending()
      : state === "ready" ? props.ready(value)
      : props.error(value)
    )],
  ]
}

// This automatically re-fetches every time you change stores, and always keeps the product
// availability count up to date when you change the filter, all with zero resource leaks.
function ProductTable({storeId}) {
  let filter = "all"
  return () => m(".item-table",
    m(".item-header", props.header),
    m(".item-filter", "Item type: ", m("select",
      m("option", {value: "all", selected: filter === "all"}, "All"),
      m("option", {value: "shirts", selected: filter === "shirts"}, "Shirts"),
      m("option", {value: "shorts", selected: filter === "shorts"}, "Shorts"),
      m("option", {value: "skirts", selected: filter === "skirts"}, "Skirts"),
      m("option", {value: "jackets", selected: filter === "jackets"}, "Jackets")
    )),

    [m(Async, {
      id: storeId,
      fetch: signal => m.request("/api/:storeId/products", {params: {storeId}, signal}),
      pending: () => m(LoadingSpinner, {variant: "large"}),
      error: e => m(ErrorDisplay, {error: e}),
      ready: ({products}) => m("table",
        m("tr",
          m("th", "Name"),
          m("th", "Description"),
          m("th", "Available")
        ),
        products
        .filter(product => filter === "all" || product.type === filter)
        .map(product =>
          m("tr", {key: item.id},
            m("td", m(m.route.Link, {href: product.id}, product.name),
            m("td", product.shortDesc),
            m("td", m(Async, {
              fetch: signal => m.request("/api/:storeId/products/:id/inventory", {
                params: {storeId, id: item.id},
                signal,
              }),
              pending: () => m(LoadingSpinner, {variant: "small"}),
              error: e => m(ErrorDisplay, {error: e}),
              ready: inventory => inventory.available,
            }))),
          )
        ),
      )
    })]
  )
}

Hopefully this clarifies where I'm coming from.

[gilbert]

Not sure if it should be different, but I think it's worth pointing out m.link usage is not consistent with m.access/m.release usage; m.link wraps a vnode, whereas the latter are children of the vnode.

[dead-claudia]

@gilbert That's true, but m.link is closer to m.fragment than m.access/m.release - it's a fragment linked to an identity (hence the name).

It does raise the question of if we should kill off m.fragment after this and other related issues, though. There's not much use for it when it's basically just m.fragment = (...args) => m.vnode.normalize(args).

[orbitbot]

Whether this is even a good idea. I like it as it's addressing actual issues I've seen over the years, but of course this is just my personal anectdata.

IMO, as a dev who uses Mithril and has done so for a fair while now already, I feel this increases the surface complexity of using Mithril enough to be a hard sell. I understand that key isn't initially intuitive, but replacing that with one or two named functions that do some "framework stuff" isn't really an improvement. Apart from an implementation complexity perspective, I guess. key can be grasped close enough as "you're using similar enough elements in a list and need to help the framework keep track of them so they work", and it's pretty much the same as in React which means that this learning transfers.

[dead-claudia]

I'll go ahead and close this for now. I'm not spending the energy to pursue this - the workaround does technically work, and there isn't really much to be gained here.