Simplify our component model

dead-claudia commented 5 years ago

Initial proposal

Based on #2293 and recent Gitter discussion, I think we could get away with simplifying our component model some in v3. **Edit:** My proposal has since expanded and got a little broader - https://github.com/MithrilJS/mithril.js/issues/2295#issuecomment-439846375. - Object components would become stateless - their `vnode.state` would just be set to `vnode.tag`. - Closure components would be kept as-is. - Class components would disappear in favor of closure components. - `vnode.oninit` would disappear in favor of closure component constructors. - I'd rename `vnode.state` to `vnode._hooks` so it's clearer it's meant to be internal. The reason for this: - Object components would then have zero memory overhead, so we could recommend them for memory optimization. - Closure components are more concise than class components, while offering more or less the same benefits of having an initialization step. - It makes no sense having an `oninit` *with* a constructor step, and with the only reason they exist (stateful object components) removed, it's kinda useless.

Here's most of the implementation changes.

This simplifies [this](https://github.com/MithrilJS/mithril.js/blob/next/render/render.js#L141-L160) function to this: ```js function initComponent(vnode, hooks) { var sentinel = vnode.tag if (typeof sentinel === "function") { if (sentinel.$$reentrantLock$$ != null) return sentinel.$$reentrantLock$$ = true vnode.state = sentinel(vnode) } else { vnode.state = sentinel } if (vnode.attrs != null) initLifecycle(vnode.attrs, vnode, hooks) initLifecycle(vnode.state, vnode, hooks) vnode.instance = Vnode.normalize(callHook.call(vnode.state.view, vnode)) if (vnode.instance === vnode) throw Error("A view cannot return the vnode it received as argument") if (typeof sentinel === "function") sentinel.$$reentrantLock$$ = null } ```

----- BTW, this route is way more flexible than React's function components (pre-hooks): - React just lets you specify the view. - This lets you specify hooks and mutate the DOM as well.

Updates

- Now a sub-bug of #2278 - Redo proposal - Fix a couple minor mistakes - Add a new section, drop `e.redraw` - Clarify `update` identity - Add ~~`m.bind`~~ `m.changed` helper - Move first proposal's archived text to the top.

This is part 2 of a broader proposal: #2278.

Let's simplify our component model for v3.

So far, every single one of our component types suck in some way:

Object components have vnode.state = Object.create(null), causing user confusion on a frequent basis when they mistakenly do new Comp(...) because this === Object.create(new Comp(...)), not this === new Comp(...). They also get really verbose because in order to avoid this issues, you're stuck with vnode.state.foo and similar, and destructuring isn't a real alternative when mutations get involved.
Class components have major this issues. React users extensively use bound class methods (method = (...args) => this.doSomething(...args)) to work around this, and it's no different with us.
Closure components get very nesting-happy, and it's very easy to forget to shadow the first vnode argument.

Also, every one of our lifecycle methods have issues:

oninit is 100% duplicated by class constructors and closure factories.
onbeforeupdate could literally be solved by a "don't redraw" sentinel + passing both parameters to the view. This also has other benefits: #2098.
onbeforeremove and onremove are basically one and the same. You could get equivalent behavior by just calling the parent onremove, awaiting it, then calling the child onremoves depth-first without awaiting them.
oncreate and onupdate are unnecessary boilerplate - we could just as easily switch to a m.update that just schedules callbacks on the next tick.¹, and you'd call this in the same place you'd render.

And finally, our component model is reeking of broken, leaky abstractions.

We have a ton of auto-redraw magic (ranging from m.request to DOM event handlers), yet we still frequently m.redraw.
For things like async loading, it's a waste to have onremove after it's loaded.
Several of us can remember the big deal over vnode.state being made no longer assignable in v2 for performance reasons.
I find myself needing the previous vnode just as often in onupdate as I do in onbeforeupdate.

Proposal

So here's my proposal: let's model components after reducers. Components would be single simple functions with four simple parameters:

The current vnode.
The previous vnode, or undefined if it's the first render.
An update function that accepts new state and schedules a global redraw.

Components would return either a vnode tree or an object with three properties:

view (required): This is the vnode tree you plan to render.
next (required): This is the vnode.state to use in the next component call.
onremove (optional): This is called when a component is removed from the DOM.

The hook onremove would be called on the parent node first, awaited from there, and then on resolution, recursively called on child elements depth-first without awaiting.

The vnode.state property would become the current state, and old.state would represent the previous state. vnode.update is how you update the current state, and it replaces the functionality of event.redraw and m.request's background: false. When called, it sets the next state synchronously and invokes m.redraw(). update itself is a constant reference stored for the lifetime of the component instance, for performance and ease of use - most uses of it need that behavior.

This would, of course, be copied over between vnode and old.

Here's how that'd look in practice:

function CounterButton({attrs, state: count = 0, update}, old) {
    if (old && attrs.color === old.attrs.color && count === old.state) return old
    return {
        next: count,
        m("button", {
            style: {color: attrs.color},
            onclick: () => update(count + 1)
        }, "Count: ", count)
    }
}

// Or, if you'd prefer arrow functions
const CounterButton = ({attrs, state: count = 0, update}, old) =>
    old && attrs.color === old.attrs.state && count === old.state ? old : {
        next: count,
        m("button", {
            style: {color: attrs.color},
            onclick: () => update(count + 1)
        }, "Count: ", count)
    }

For comparison, here's what you'd write today:

function CounterButton() {
    let count = 0
    let updating = false

    return {
        onbeforeupdate(vnode, old) {
            if (updating) { updating = false; return true }
            return vnode.attrs.color !== old.attrs.color
        },

        view(vnode) {
            return m("button", {
                style: {color: vnode.attrs.color},
                onclick: () => { updating = true; count++ }
            }, "Count: ", count)
        },
    }
}

Attributes

If I'm going to ditch lifecycle attributes, I'll need to come up with similar for elements. This will be a single magic attribute onupdate like the above, with the same vnode and old, but returned vnode trees and returned state are ignored - you can only diff attributes. In addition, the third update argument does not exist. There's reasons for ignoring each:

Vnode children should be specified in the children, not the hook.
It's unclear whether the state should be for the attributes only or if it should receive the vnode state. And even in that case, it seems wrong for you to be able to replace it because it'd break encapsulation.

You can still return old or {view: old, ...} to signify "don't update this vnode". That's the only reason view is ever read.

Mapping

The hooks would be fused into this:²

oninit would become just the first component call. The component would be rendered as tag(vnode, undefined, undefined, update), and the first render can be detected by the previous vnode being undefined.
oncreate would become invoking m.update in the first component call.
onbeforeupdate would be conditionally returning old from the component.
view would be returning the vnode tree from either the top level or via the view property.
onupdate would be invoking m.update on subsequent component calls.
onbeforeremove would be optionally returning a promise from onremove.
onremove would be returning a non-thenable from onremove.

Helpers

There are two new helpers added: m.update and m.changed. m.update is literally just sugar for scheduling a callback to run on the next tick, but this is practically required for DOM manipulation. m.bind dramatically simplifies things like subscription management, by implicitly cleaning things up for you as necessary.

m.changed is a simple helper function you call as const changed = m.changed(state, key, prevKey), and is a cue for you to (re-)initialize state. For convenience, m.changed(state, vnode, old, "attr") is sugar for m.changed(state, vnode.attrs[attr], old && old.attrs[attr]), since the common use here deals with attributes.

When this returns false, you should continue using state as necessary.
When this returns true, it invokes state.close() if such a method exists and signals to you that you should reinitialize it.
For convenience, when state == null, this returns true.

Why this?

I know this is very different from the Mithril you knew previously, but I have several reasons for this:

Hooks vs reducer

When you start looking into how the hooks work conceptually, it stops being as obvious why they're separate things.

oncreate/onupdate are always scheduled after every call to view, and the code to explicitly schedule them is sometimes simpler than defining the hooks manually. In this case, you might as well put them in the view directly - you'll save Mithril quite a bit of work.
The first render with v1/v2 is always oninit > view > schedule oncreate. If you couple the current state to the current view, you can merge this entire path.
Subsequent renders with v1/v2 are always onbeforeupdate > view > schedule onupdate. If you provide the previous attributes, allow returning a "don't update the view" sentinel, and couple the current state to the current view, you can merge this entire path.
Usually, you're doing mostly the same thing regardless of if it's the first render or a subsequent one. And when you're not, the differences are usually pretty small.
By always providing the old and new vnodes, it's much easier to react to differences without having to keep as much state.

And so because they are so similar, I found it was much simpler to just reduce it to a single "reducer" function. For the first render, it's pretty easy to check for a missing previous vnode (old == null).

If you'd like to see how this helps simplify components:

This non-trivial, somewhat real-world example results in about 25% less code. (It's a component wrapper for [Bootstrap v4's modals](https://getbootstrap.com/docs/4.1/components/modal/).) ```js // v1/v2: 44 lines function Modal(vnode) { let showing = !!vnode.attrs.show return { oncreate: vnode => { $(vnode.dom).modal(vnode.attrs.options) if (showing) $(vnode.dom).modal("show") else $(vnode.dom).modal("hide") }, onupdate: vnode => { const next = !!vnode.attrs.show if (showing !== next) { showing = next $(vnode.dom).modal("toggle") } }, onremove: vnode => { $(vnode.dom).modal("dispose") }, view: vnode => m(".modal[tabindex=-1][role=dialog]", vnode.attrs.modalAttrs, [ m(".modal-dialog[role=document]", { class: vnode.attrs.centered ? "modal-dialog-centered" : "", }, [ m(".modal-content", [ m(".modal-header", vnode.attrs.headerAttrs, [ m(".modal-title", vnode.attrs.titleAttrs, vnode.attrs.title), vnode.attrs.header, ]), m(".modal-body", vnode.attrs.bodyAttrs, vnode.attrs.body), m(".modal-footer", vnode.attrs.footerAttrs, vnode.attrs.footer) ]) ]) ]) } } Modal.Dismiss = {view: ({attrs, children}) => { let tag = attrs.tag || "button[type=button].close" tag += "[data-dismiss=modal][aria-label=Close]" return m(tag, attrs.attrs, children) }} // This proposal: 32 lines function Modal(vnode, old) { if (!old || !!vnode.attrs.show !== !!old.attrs.show) { m.update(() => { if (!old) $(vnode.dom).modal(vnode.attrs.options) $(vnode.dom).modal(vnode.attrs.show ? "hide" : "show") }) } return { onremove: () => $(vnode.dom).modal("dispose"), view: m(".modal[tabindex=-1][role=dialog]", vnode.attrs.modalAttrs, [ m(".modal-dialog[role=document]", { class: vnode.attrs.centered ? "modal-dialog-centered" : "" }, [ m(".modal-content", [ m(".modal-header", vnode.attrs.headerAttrs, [ m(".modal-title", vnode.attrs.titleAttrs, vnode.attrs.title), vnode.attrs.header ]), m(".modal-body", vnode.attrs.bodyAttrs, vnode.attrs.body), m(".modal-footer", vnode.attrs.footerAttrs, vnode.attrs.footer) ]) ]) ]) } } Modal.Dismiss = ({attrs, children}) => { let tag = attrs.tag || "button[type=button].close" tag += "[data-dismiss=modal][aria-label=Close]" return m(tag, attrs.attrs, children) } ```

State

I chose to make state in this iteration with a heavy preference for immutability because of two big reasons:

It's much easier to use and reason about with immutable state.
You can diff the state in a central place, if you treat it immutably. This is one of the few useful things React has.

I also coupled it to the view so it's easier to diff and update your state based on new attributes without having to mutate anything.

It's not completely anti-mutation, and it's not nearly as magical as React's setState - it's more like their replaceState. You can still do mutable things in it and it will work - you could take a look at the Media component definition here³. It just makes it clear that in smaller scenarios, there may be easier ways to do it, and it encourages you to do things immutably with smaller state. For large, stateful components, it's still often easier to just mutate state directly and do vnode.update(state) - you get the benefits of partial updates without the cost of constantly recreating state.

No auto-redraw anymore?

Generally not. But because update both replaces the state and auto-redraws,

Motivation

My goal here is to simplify Mithril's component model and help bring it back to that simplicity it once had. Mithril's components used to only consist of two properties in v0.2: a controller constructor and a view(ctrl, ...params): vnode method. You'd use this via m(Component, ...params), and it would be fairly consistent and easy to use.⁴

It also had far fewer lifecycle hooks - it only had two: attrs.config(elem, isInit, ctx, vnode) for most everything that requires DOM integration and ctrl.onunload(e)/ctx.onunload(e) for what we have onremove for. If you wanted to do transitions, you'd do m.startComputation() before you trigger it and m.endComputation() after it finishes, and this would even work in onunload. If you wanted to block subtree rendering, you'd pass {subtree: "retain"} as a child.⁵

I miss that simplicity in easy-to-understand component code, and I want it back. I want all the niceness of v0.2's components without the disappointment.

Notes

m.update would literally be this. It's pretty easy, and it's incredibly easy to teach. It's also simpler to implement than even m.prop or m.withAttr.
```
var promise = Promise.resolve()
m.update = function (callback) { promise.then(callback) }
```

For a more concrete explanation of how hooks correspond to this, here's how I could wrap legacy v1/v2 components for migration:

function migrateComponent(tag) {
    const makeSynthetic = ({
        key, attrs, children, text, dom, domSize
    }, state) => ({
        tag, key, attrs, children, text, dom, domSize, state,
    })

    return (vnode, old) => {
        let current, test
        if (old == null) {
            current = makeSynthetic(vnode, undefined)
            if (typeof tag !== "function") {
                current.state = Object.create(tag)
            } else if (tag.prototype && typeof tag.prototype.view === "function") {
                current.state = new tag(current)
            } else {
                current.state = tag(current)
            }
            current.state.oninit(current)
            m.update(() => current.state.oncreate(current))
        } else {
            const prev = vnode.state
            current = makeSynthetic(vnode, vnode.state.state)
            test = current.state.onbeforeupdate(vnode.state, current)
            if (test == null || test) m.update(() => current.state.onupdate(current))
        }

        return {
            next: current,
            view: test == null || test ? current.state.view(current) : old,
            onremove() {
                const result = current.state.onbeforeremove(current)
                if (result && result.then === "function") {
                    return Promise.resolve(result).then(() => {
                        current.state.onremove(current)
                    })
                } else {
                    current.state.onremove(current)
                }
            }
        }
    }
}

Here's the source code for the Media component, used in an example in https://github.com/MithrilJS/mithril.js/issues/2281. It's basically a stripped-down port of react-media.

// Okay, and this is why I like Mithril: it's not super complicated and full of
// ceremony to do crap like this.
function Media(vnode, old) {
    let queryList = vnode.state

    if (m.changed(queryList, vnode, prev, "query")) {
        let mediaQueryList = window.matchMedia(query)
        // Safari doesn't clean up the listener with `removeListener` when the
        // listener is already waiting in the event queue. The `mediaQueryList`
        // test is to make sure the listener is not called after we call
        // `removeListener`.
        queryList = {
            matches: () => mediaQueryList.matches,
            cleanup: () => {
                mediaQueryList.removeListener(updateMatches)
                mediaQueryList = undefined
            }
        }
        function updateMatches() { if (mediaQueryList) m.redraw() }
        mediaQueryList.addListener(updateMatches)
    }

    return {
        next: queryList,
        view: vnode.attrs.view(queryList.matches()),
        onremove: queryList.cleanup,
    }
}

Well...not always easy. Mithril v0.2 didn't normalize children to a single array, so proxying children got rather inconvenient at times. What this means is that if you used m(Comp, {...opts}, "foo", "bar"), the component's view would literally be called as Comp.view(ctrl, {...opts}, "foo", "bar"), so you'd often need to normalize children yourself. It also didn't help that this was all pre-ES6, or it would've been a little more tolerable.
Most of the internal complexity and bugs in v0.2 were caused indirectly by one of two things: the spaghetti mess of a diff algorithm and m.redraw.strategy() causing hard-to-debug internal state issues thanks to constantly ruined assumptions. Oh, and insult to injury: e.preventDefault() in ctrl.onunload or ctx.onunload means you can't even assume unmounting the tree clears it or that route changes result in a fresh new tree anywhere. It's all these kinds of papercuts caused by internal bugs in a seriously messy code base that led Leo to rewrite and recast the entire API for v1.

dead-claudia commented 5 years ago

This also kind of relates to #2278.

barneycarroll commented 5 years ago

@isiahmeadows thanks. What's the use case for maintaining vnode.state at all in this scenario? I'd rather have unwitting victims of breaking change during upgrade see an error when they attempt to access it, rather than find an entity which looks similar but fails to behave as they expect.

dead-claudia commented 5 years ago

@barneycarroll That's what would happen. I'm proposing renaming the property so it's clearly not. As for why I'm not doing away with the value itself, we need the instance somewhere for our own purposes, even if it's in something like vnode._hooks (that's clearly private).

StephanHoyer commented 5 years ago

Maybe we could add a deprecation warning in v2 when accessing vnode.state

dead-claudia commented 5 years ago

@StephanHoyer 99% of the migration for this could be done before upgrading Mithril, so I'm not convinced it's necessary. If this change were implemented for v3, here's how the migration would go:

Switch all class components and stateful object components to closure components. This would remove 99% of vnode.state uses.
Audit all remaining uses of vnode.state (search for .state) and change them to call local functions as necessary. This would amount to very few if any cases, so you could do this one at a time.
Swap all uses of m() to something like the factory below and fix any errors caught.
Upgrade to Mithril v3
Fix any errors that may have arisen (mostly just things missed from the first two steps).

The last two steps would entail very little effort compared to the first three, since those would catch 99% of your mistakes.

// This is probably broken and it obviously won't work on IE or other ES5 environments.
function m(tag, ...rest) {
    if (typeof tag !== "string") {
        if (typeof tag === "function" && tag.prototype && tag.prototype.view) {
            throw new Error("Class components are no longer supported!")
        } else if (typeof tag !== "function" && !tag.$$wrapped) {
            tag.$$wrapped = true
            const {oninit, onbeforeupdate} = tag
            const lock = vnode => {
                if (vnode.$$locked != null) return
                vnode.$$locked = false
                let state = vnode.state
                Object.defineProperty(vnode, "state", {
                    get() {
                        if (vnode.$$locked) throw new ReferenceError("`vnode.state` is deprecated!")
                        return state
                    },
                    set(s) { state = s },
                })
            }
            ;["oninit", "oncreate", "onupdate", "onbeforeremove", "onremove"]
            .forEach(name => {
                const method = tag[name]
                tag[name] = function (vnode) {
                    lock(vnode)
                    vnode.$$locked = true
                    try {
                        return method.apply(this, arguments)
                    } finally {
                        vnode.$$locked = false
                    }
                }
            })
            tag.onbeforeupdate = function (vnode, old) {
                lock(vnode); lock(old)
                vnode.$$locked = old.$$locked = true
                try {
                    if (onbeforeupdate) return onbeforeupdate.apply(this, arguments)
                } finally {
                    vnode.$$locked = old.$$locked = false
                }
            }
        }
        return oldM(tag, ...rest)
    }
}

nordfjord commented 5 years ago

I wonder whether we could add a third type of component

Vnode -> () -> Vnode

That is to say if a closure component returns a function, it's treated as a view.

const simpleCounter = (vnode) => {
  let count = vnode.attrs.count || 0;

  return ()=> m('', [
    m('h1', `The count is: ${count}`),
    m('button', {onclick: ()=> ++count}, '+'),
    m('button', {onclick: ()=> --count}, '-'),
  ])
}

I'm finding that in most cases I use a closure component, I'm just returning an object with a view function.

This would mean a change to the initComponent method

if (typeof sentinel === "function") {
  if (sentinel.$$reentrantLock$$ != null) return
    sentinel.$$reentrantLock$$ = true
    const view = sentinel(vnode)
    vnode.state = typeof view === 'function' ? {view} : view
}

CreaturesInUnitards commented 5 years ago

I love this idea, if indeed it doesn't have more wide-ranging repercussions

dead-claudia commented 5 years ago

@nordfjord I don't see anything different between that and this:

 const simpleCounter = (vnode) => {
   let count = vnode.attrs.count || 0;

-  return        ()=> m('', [
+  return {view: ()=> m('', [
     m('h1', `The count is: ${count}`),
     m('button', {onclick: ()=> ++count}, '+'),
     m('button', {onclick: ()=> --count}, '-'),
-  ])
+  ])}
 }

It's literally 8 extra characters per component, and I'm convinced it's worth the added complexity. (The goal of this proposal is to simplify the model, not to add yet another component type.)

spacejack commented 5 years ago

I'm pretty much on board with this proposal. But to play devil's advocate:

Is the lack of class components a liability for adoption? A lot of people think in classes. Typescript TSX types can't yet handle closure components. i.e. <ClosureComponent /> won't work. Perhaps this will be resolved before this change would happen.

Browser vendors seem to be more aggressively optimizing classes over closures. While this is almost never going to be an issue for stateful components, would it ever desirable to have a higher performing stateful component with less overhead than a closure?

The thing I am still more attached to is oninit. Though not necessary I still find it convenient in closure components. Here are a few examples:

Convenient async oninit:

function Comp() {
    let item
    return {
        async oninit(v) {
            item = await m.request('/api/item' + v.attrs.id)
        },
        view() {...}
    }
}

A pitfall without oninit:

function Comp(v) {
    const initialFoo = v.attrs.foo
    return {
        view() {
            // Using stale vnode by mistake
            return m('ID: ' + v.attrs.id)
        }
    }
}

Alternately, to prevent accidental re-use and to garbage-collect the initial vnode:

function Comp(v) {
    const initialFoo = v.attrs.foo
    v = null
    return {
        view() {
            // Will throw an obvious error
            return m('ID: ' + v.attrs.id)
        }
    }
}

Typescript use case for oninit:

export default function Comp(): m.Component<Attrs> {
    let item: Item | undefined
    return {
        async oninit(v) {
            item = await m.request('/api/item/' + v.attrs.id)
        },
        view(v) {
            // ...
        }
    }
}

More verbose and awkward without:

const Comp: m.FactoryComponent<Attrs> = function(v) {
    let item: Item | undefined
    async function init() {
        item = await m.request('/api/item/' + v.attrs.id)
        v = undefined as any
    }
    init()

    return {
        view(v) {
            // ...
        }
    }
}

export default Comp

There might also be some use cases for affecting global state in a POJO component's oninit hook rather than in oncreate.

CreaturesInUnitards commented 5 years ago

@isiahmeadows @spacejack I don't see the benefit to ripping oninit out of closures. I also can't see myself using it, but some things are merely a matter of taste. And I have to say that it's nice to be able to grab a POJO and paste it inside a closure wholesale with no adjustments needed.

dead-claudia commented 5 years ago

@CreaturesInUnitards

I don't see the benefit to ripping oninit out of closures. I also can't see myself using it, but some things are merely a matter of taste. And I have to say that it's nice to be able to grab a POJO and paste it inside a closure wholesale with no adjustments needed.

The main reason why I want to rip it out is because it's just duplicated functionality.

Object components have oninit because you can't observe Object.create(object) calls from object itself.
Closure components and class components have that initialization step already in the form of function invocation and class instantiation, respectively.

If we rip out the Object.create step for objects, now the oninit is just initializing global state each time a component is allocated. And for closure and class components, it overlaps almost completely with just allocating the instance.

@spacejack You just gave me an idea here: how about we just drop the initial vnode parameter for closure and class components altogether. I couldn't tell you how many times I've seen pop up in Gitter people having issues with them because they're forgetting to include the vnode as a hook parameter. We'd instead keep oninit in this scenario, as a "first time you receive a vnode".

dead-claudia commented 5 years ago

Edit: Use previous vnode instead of a new m.retain for the sentinel.

@spacejack @CreaturesInUnitards I just came up with an alternate way that could simplify components further:

oninit, onbeforeupdate, and view would be fused into view(vnode, old), where old === undefined on the first render.
- Return old to prevent subtree update. This works not unlike Mithril v0.2's old {subtree: "retain"}, and it'd replace onbeforeupdate's functionality.
- When you return old on first render (when it's undefined), it's equivalent to an empty view, so on an empty tree, it's basically equivalent to not updating the subtree.
oncreate(vnode), onupdate(vnode), and onremove(vnode) would all fuse into a single shared onupdate(vnode, old), where old === undefined on create and vnode === undefined on remove.
- When old is not undefined and is returned from the view, this hook is not fired after the update.
onbeforeremove(vnode) would remain as-is.
If a view attribute exists on a vnode:
- The state is updated as normal.
- Existing children are cleared on the new vnode.
- The view is invoked, and its result used as the vnode's children.
- The vnode is then rendered with the new children. (Components would have to use something other than view for their method name.)
The lifecycle attrs on vnodes and components would become:
- onupdate(vnode, old): void
- view(vnode, old): vnode
- onbeforeremove(vnode): any | Promise<any>
Components would take two primary forms:
- Object components: {view, onupdate?, onbeforeremove?}
- Closure components: () => {view, onupdate?, onbeforeremove?}

There are several benefits of this:

view being callable from attrs with its result used would solve #2050.
Always receiving both the old and new attrs would solve #2098.
onupdate being passed both the old and new attrs would dramatically reduce how much state you need in case you need to diff attrs for third-party integration.
view being passed both the old and new attrs would make it easier for you to tweak your returned view at a much more fine-grained level, and it requires less state to sustain.
This cuts the list of related lifecycle methods from 6 to 3, making internal optimization a bit easier. For example, I could experiment with this on both attributes and vnodes without blowing up memory usage.
Not passing attrs to the closure constructor makes for an easier experience with TS.
Best practices become a little more obvious and easier to teach:
- If it's DOM-related, it goes in onupdate.
- If it's view-related, it goes in view.
- You can't forget to shadow a variable that doesn't exist. Accidentally reusing the closure's vnode parameter is a common gotcha that would no longer exist with this.
- Stateful views become even bigger of a red flag, especially if they're not guarded with at least something like if (vnode.attrs.id !== prev.attrs.id).
- It becomes much easier to react to state changes when you always receive the old and new attrs, to the point it's often simpler.
- It encourages you even more to keep your component updated using attributes rather than internal state.
- vnode.dom and vnode.domSize are uninitialized in the view, but you otherwise never receive a vnode with uninitialized data.
React has been experimenting a bit with a nice little DSL, and I feel we could probably gain most of the benefits (less code duplication, simpler integration, fewer gotchas) with few of the drawbacks (like their iffy DSL and counter-intuitive composition semantics) if we just fused all our hooks together.
- I think Mithril v0.2 was on to something by keeping it simple - its config attribute at its core was fundamentally pretty sound, just its design was really ad-hoc. The v1 rewrite really felt like both a step forward (better decomposition) and a step back (more complicated API), and I'd like for us to recover that lost step by reigning in the API complexity. I miss that beautiful simplicity it had.

Comparison

To draw from [a couple of React's](https://reactjs.org/docs/hooks-effect.html) [hooks API examples](https://reactjs.org/docs/hooks-custom.html), here's how this would compare: ```js // v2 function FriendWithStatusCounter() { let count = 0 let isOnline, prevId function onChange(status) { isOnline = status.isOnline; m.redraw() } return { oncreate({attrs}) { document.title = `You clicked ${count} times` ChatAPI.subscribeToFriendStatus(attrs.friend.id, onChange) prevId = attrs.friend.id }, onupdate({attrs}) { document.title = `You clicked ${count} times` if (prevId !== attrs.friend.id) { ChatAPI.unsubscribeFromFriendStatus(prevId, onChange) ChatAPI.subscribeToFriendStatus(attrs.friend.id, onChange) prevId = attrs.friend.id } }, view: () => isOnline === null ? 'Loading...' : isOnline ? 'Online' : 'Offline', } } // This proposal function FriendWithStatusCounter() { let count = 0 let isOnline function onChange(status) { isOnline = status.isOnline; m.redraw() } return { onupdate({attrs} = {}, {attrs: prev} = {}) { document.title = `You clicked ${count} times` // Lodash lets me do optional chaining if ((attrs && attrs.friend.id) !== (prev && prev.friend.id)) { if (prev) ChatAPI.unsubscribeFromFriendStatus(prev.friend.id, onChange) if (attrs) ChatAPI.subscribeToFriendStatus(attrs.friend.id, onChange) } }, view: () => isOnline === null ? 'Loading...' : isOnline ? 'Online' : 'Offline', } } ``` If you wanted to use a helper, similar to what React allows with custom hooks, it still composes reasonably well: ```js // v2 + helper class FriendStatus { constructor() { this.prevId = this.isOnline = undefined this.onChange = status => { this.isOnline = status.isOnline; m.redraw() } } update(friend) { if (this.prevId === friend.id) return if (this.prevId != null) { ChatAPI.unsubscribeFromFriendStatus(this.prevId, this.onChange) } if (friend.id != null) ChatAPI.subscribeToFriendStatus(friend.id, this.onChange) this.prevId = friend.id } } function FriendWithStatusCounter() { const status = new FriendStatus() let count = 0 return { oncreate({attrs}) { document.title = `You clicked ${count} times` status.update(attrs.friend.id) }, onupdate({attrs}) { document.title = `You clicked ${count} times` status.update(attrs.friend.id) }, view: () => status.isOnline === null ? 'Loading...' : status.isOnline ? 'Online' : 'Offline', } } // This proposal + helper class FriendStatus { constructor() { this.isOnline = undefined this.onChange = status => { this.isOnline = status.isOnline; m.redraw() } } update(friend, prev) { if (prev.id === friend.id) return if (prev.id != null) ChatAPI.unsubscribeFromFriendStatus(prev.id, this.onChange) if (friend.id != null) ChatAPI.subscribeToFriendStatus(friend.id, this.onChange) } } function FriendWithStatusCounter() { const status = new FriendStatus() let count = 0 return { onupdate(vnode, prev) { if (vnode) document.title = `You clicked ${count} times` status.update(vnode && vnode.attrs.friend, prev && prev.attrs.friend) }, view: () => status.isOnline === null ? 'Loading...' : status.isOnline ? 'Online' : 'Offline', } } ``` You can see that the helper is also considerably simpler.

@spacejack

Typescript TSX types can't yet handle closure components. i.e. won't work. Perhaps this will be resolved before this change would happen.

As I mentioned tongue-in-cheek on Gitter, TypeScript is already running into problems of their own making from hard-coding the types of React's classes, stateless functional components, and elements into the compiler rather than making them configurable - React 16 changed these types substantially, and TS already (mostly) supports attributes. So I expect TypeScript to resolve their end eventually. (And as slow as Mithril's historically evolved, they would likely beat us.)

barneycarroll commented 5 years ago

@isiahmeadows when submitting issues consisting of long nested lists, I'd appreciate numbering in order to be able to reference things specifically - otherwise discussing the detail becomes very difficult.

Add a new vnode.tag === "!" that works similarly to Mithril v0.2's {subtree: "retain"}. This would be a proper vnode, and users would construct it via m(m.retain, attrs).

m would literally ignore any and all children it has if tag === "!".

If key === undefined, this just returns a memoized instance.

This belongs in user-land. The existing API can easily be adapted to cater for this:

const Static = initial => ({
  view: () => initial.children,
})

// used as

m(Static, 
  memoizedContents,
)

// alternatively

m.fragment({
  onbeforeupdate: () => false,
},
  memoizedContents,
)

I'm glad #2098 is getting traction. But as regards changing the behaviour on onupdate: if the proposal for a unified post-draw hook were to go ahead, I would prefer a new name altogether (onafterview?), and for the ability to use the existing hooks remain for convenience.

But it strikes me that we may not need the hook at all. Part of the justification for the magic of React's new hooks rests on React's opinionated draw pipeline. We don't have that burden to satisfy and currently we can guarantee that, when a view is executing, the DOM will be ready on the next tick. This in mind we can fold all eventualities into a 2-hook system

{
  view: (now, then) => {
    if(!then){ // first pass
      oninit()

      setTimeout(oncreate)

      return initialView
    }
    else { // subsequent passes
      onbeforeupdate()

      setTimeout(onupdate)

      return then.children // <= onbeforeupdate: () => false

      return furtherView
    }

    // universal
    setTimeout(onafterview)

    return view
  },

  onbeforeremove: (now, then?) => {
    await onbeforemove()

    onremove()
  },
}

dead-claudia commented 5 years ago

@barneycarroll

when submitting issues consisting of long nested lists, I'd appreciate numbering in order to be able to reference things specifically - otherwise discussing the detail becomes very difficult.

My bad. It was basically just a giant brain dump. I updated it to be numbered now.

This belongs in user-land. The existing API can easily be adapted to cater for this:

I'm suggesting replacing the existing API, not adding to it. They're technically equivalent, and that's the point. But the reason I'm suggesting replacing it is because of what it unlocks: the ability to use the diff to create the view without using vnode state.

Edit: Now that I read what you were saying correctly, I think you misunderstood what I was saying. That's intended to be a primitive component, down the vein of what was discussed in #2279. I've since updated the suggestion to use the old vnode instead of a special vnode, per your suggestion. (I altered it for reasons explained below.)

But it strikes me that we may not need the hook at all. Part of the justification for the magic of React's new hooks rests on React's opinionated draw pipeline. We don't have that burden to satisfy and currently we can guarantee that, when a view is executing, the DOM will be ready on the next tick. This in mind we can fold all eventualities into a 2-hook system

Mithril has an almost identical draw pipeline to React's pre-Fibers, and so does nearly every other virtual DOM library. Also, we don't have a reference to the DOM until after the returned tree is rendered, so you can't fuse the view and onupdate.

I did consider dropping onbeforeremove and just letting onupdate return an awaited promise (ignored on subtrees) - it's basically the same phase. My concern is how to signal to the child component it should ignore transitions, but since that's basically the 99% use case for it, I'm thinking of just letting it attempt and letting the callback get collected. We could just let the docs note this.

Your return then.children (my prev.children) seems really compelling... I'd prefer returning prev and checking that over prev.children, so it's a little more obvious and a little less ambiguous. If you really want a vnode tree, we could compare against prev.instance, but that comes at the risk of exposing what's currently a private implementation detail. It also makes it impossible to attempt the nonsensical return m(m.retain) on first render.

barneycarroll commented 5 years ago

Mithril has an almost identical draw pipeline to React's pre-Fibers, and so does nearly every other virtual DOM library. Also, we don't have a reference to the DOM until after the returned tree is rendered, so you can't fuse the view and onupdate.

@isiahmeadows if you destructure the vnode in the arguments, then yes. But if you only query vnode.dom when the timeout resolves, it will be there. This works out of the box as we speak:

m.mount(document.body, {
  view: v => {
    setTimeout(() => {
      v.dom.style = 'color: red'
    })

    return m('p', 'Hello!')
  }
})

I did consider dropping onbeforeremove and just letting onupdate return an awaited promise (ignored on subtrees) - it's basically the same phase. My concern is how to signal to the child component it should ignore transitions, but since that's basically the 99% use case for it, I'm thinking of just letting it attempt and letting the callback get collected. We could just let the docs note this.

Could you elaborate on this? I think you're referring to the problem of how to trigger exit animations in descendants of a removed node. This can be achieved in user-land.

Your return then.children (my prev.children) seems really compelling... I'd prefer returning prev and checking that over prev.children, so it's a little more obvious and a little less ambiguous. If you really want a vnode tree, we could compare against prev.instance, but that comes at the risk of exposing what's currently a private implementation detail. It also makes it impossible to attempt the nonsensical return m(m.retain) on first render.

Well observed - the right thing to do would be to interpolate vnode.instance, but since the equality check isn't fired on instances (because, as you say, they're supposed to be private), it doesn't behave as desired - if I monkey-patch the (now, then) functionality onto m, the only way to get this to work currently is to ensure the view returns a fragment, and then to explicitly interpolate vnode.instance.children if we want memoization. So is your proposal that we allow the returning of vnodes from components to trigger this behaviour? This would be odd considering returning the current vnode is still a very ambiguous instruction. Or shall we say current is good enough?

dead-claudia commented 5 years ago

@barneycarroll

if you destructure the vnode in the arguments, then yes. But if you only query vnode.dom when the timeout resolves, it will be there.

That counts as "after the returned tree is rendered" - you'd get a similar result if you simply did Promise.resolve().then(() => { v.dom.style = "color: red" }). 😉

Could you elaborate on this? I think you're referring to the problem of how to trigger exit animations in descendants of a removed node.

I was talking about the class of problems solved by having onbeforeremove, which IMHO seems incredibly limited. It also doesn't really seem to be usefully separated from onremove.

Transitions require you to await them, hence why onbeforeremove exists.
Most things in onremove could be done while awaiting onbeforeremove - it doesn't depend on resolution.
If it really does depend on resolution (like if you need to unregister an element from a third party plugin after a transition), you could just do awaitTransition(vnode.dom).then(() => { cleanupPlugin(vnode.dom) }).

So is your proposal that we allow the returning of vnodes from components to trigger this behaviour?

No, just the ability to return the current vnode (the one we're updating). Anything else would still continue to throw an error, including returning the updated one.

barneycarroll commented 5 years ago

That counts as "after the returned tree is rendered" - you'd get a similar result if you simply did Promise.resolve().then(() => { v.dom.style = "color: red" }). 😉

Yes of course, but my point remains - you can do away with onupdate via this mechanism.

The idea occurred to me when thinking about how spurious React's new hooks are - essentially strange exotic first class entities based on timing conditions. As we've determined WRT the issue of DOM paint ticks in onupdate, these subtleties can be perfectly easily addressed in user-land.

I agree with your assessment that onremove is technically redundant given the existence of onbeforeremove. There is a deep semantic beauty in being able to distinguish synchronous / asynchronous teardown at a high level though - and I also have a pet fear of functions which are conditionally async 😬

No, just the ability to return the current vnode (the one we're updating). Anything else would still continue to throw an error, including returning the updated one.

Show me what the desired user-land code would look like?

dead-claudia commented 5 years ago

@barneycarroll

Yes of course, but my point remains - you can do away with onupdate via this mechanism.

The idea occurred to me when thinking about how spurious React's new hooks are - essentially strange exotic first class entities based on timing conditions. As we've determined WRT the issue of DOM paint ticks in onupdate, these subtleties can be perfectly easily addressed in user-land.

It's still useful to at least schedule in the current tick for after rendering. And if we don't offer that hook, m.render is no longer guaranteed to be synchronous assuming no async onbeforeremove (or equivalent) calls need awaited.

But also, I'd rather not rely on users having to do some serious hack just to schedule some work. We could solve this via an update(callback, {...args}) callback as a third argument, and we'd by default use a local hook list, but for m.mount/m.redraw, we'd want to use the global hooks list so we can just call them all at once.

This would mean we could change the component instance to just a plain render function:

render(vnode, old, update):
- On first render, old is undefined.
- On final render, vnode is undefined. If you return a thenable, it's awaited, but the return value is otherwise ignored.
- update(callback) enqueues a callback with an optional argument to call after the subtree is rendered.
- Return old when vnode exists to prevent rendering any subtree.
Elements would accept a single magic attribute render which would be a function like above.
- Returned vnode trees are ignored here. Vnodes are only rendered with components.

When attempting the first render, we'd differentiate pure instances from stateful components when invoking the component the first time. If it returns a function, we treat that as the instance and invoke it every time. If it returns a tree, we treat the component itself as the instance and just use the initial tree.

Here's what that'd end up looking like:

```js // v2 function FriendWithStatusCounter() { let count = 0 let isOnline, prevId function onChange(status) { isOnline = status.isOnline; m.redraw() } return { oncreate({attrs}) { document.title = `You clicked ${count} times` ChatAPI.subscribeToFriendStatus(attrs.friend.id, onChange) prevId = attrs.friend.id }, onupdate({attrs}) { document.title = `You clicked ${count} times` if (prevId !== attrs.friend.id) { ChatAPI.unsubscribeFromFriendStatus(prevId, onChange) ChatAPI.subscribeToFriendStatus(attrs.friend.id, onChange) prevId = attrs.friend.id } }, view: () => isOnline === null ? 'Loading...' : isOnline ? 'Online' : 'Offline', } } // This proposal function FriendWithStatusCounter() { let count = 0 let isOnline function onChange(status) { isOnline = status.isOnline; m.redraw() } return ({attrs} = {}, {attrs: prev} = {}) => { document.title = `You clicked ${count} times` if ((attrs && attrs.friend.id) !== (prev && prev.friend.id)) { if (prev) ChatAPI.unsubscribeFromFriendStatus(prev.friend.id, onChange) if (attrs) ChatAPI.subscribeToFriendStatus(attrs.friend.id, onChange) } return isOnline === null ? 'Loading...' : isOnline ? 'Online' : 'Offline' } } ``` And for your example: ```js return (vnode, prev, update) => { if (!prev) { // first pass oninit() update(oncreate, vnode) return initialView } else if (!vnode) { // last pass const result = onbeforeremove(prev) if (typeof result.then !== "function") update(onremove, vnode) else result.then(() => update(onremove), () => update(onremove, vnode)) } else if (!onbeforeupdate(vnode, prev)) ( { // subsequent passes return prev } else { update(onupdate, vnode) return furtherView } } ```

I agree with your assessment that onremove is technically redundant given the existence of onbeforeremove. There is a deep semantic beauty in being able to distinguish synchronous / asynchronous teardown at a high level though - and I also have a pet fear of functions which are conditionally async 😬

I can sympathize, and conditionally async is usually a PITA. Just in this case, it's much easier to use if we do allow conditionally async behavior.

Show me what the desired user-land code would look like?

Something like this:

// v2
function CounterButton() {
    let count = 0
    let updating = false

    return {
        onbeforeupdate(vnode, old) {
            if (updating) { updating = false; return true }
            return vnode.attrs.color !== old.attrs.color
        },

        view(vnode) {
            return m("button", {
                style: {color: vnode.attrs.color},
                onclick: () => { updating = true; count++ }
            }, "Count: ", count)
        },
    }
}

// The `view`/`onupdate`/`onbeforeremove` proposal
function CounterButton() {
    let count = 0
    let updating = true

    return {view(vnode, old) {
        if (!vnode) return undefined
        if (updating) updating = false
        else if (vnode.attrs.color === old.attrs.color) return old

        return m("button", {
            style: {color: vnode.attrs.color},
            onclick: () => { updating = true; count++ }
        }, "Count: ", count)
    }}
}

// The `render` function proposal
function CounterButton() {
    let count = 0
    let updating = true

    return (vnode, old) => {
        if (!vnode) return undefined
        if (updating) updating = false
        else if (vnode.attrs.color === old.attrs.color) return old

        return m("button", {
            style: {color: vnode.attrs.color},
            onclick: () => { updating = true; count++ }
        }, "Count: ", count)
    }
}

We could take the component-as-a-function one step further and make it a proper reducer, making components and render attrs (vnode, old, update, state) => view | {next, view} functions where state is undefined on first render. If no next is returned (as in, either an object with a tag or a non-object), state is undefined on the next call. This would make it very much like React's hooks, just with fewer downsides and a lot less magic.

Here's what that'd end up looking like:

- v2 ```js function Modal(vnode) { let showing = !!vnode.attrs.show return { oncreate(vnode) { $(vnode.dom).modal(vnode.attrs.options) if (showing) $(vnode.dom).modal("show") else $(vnode.dom).modal("hide") }, onupdate(vnode) { const next = !!vnode.attrs.show if (showing !== next) { showing = next $(vnode.dom).modal("toggle") } }, onremove(vnode) { $(vnode.dom).modal("dispose") }, view: vnode => m(".modal[tabindex=-1][role=dialog]", vnode.attrs.modalAttrs, [ m(".modal-dialog[role=document]", { class: vnode.attrs.centered ? "modal-dialog-centered" : "", }, [ m(".modal-content", [ m(".modal-header", vnode.attrs.headerAttrs, [ m(".modal-title", vnode.attrs.titleAttrs, vnode.attrs.title), vnode.attrs.header, ]), m(".modal-body", vnode.attrs.bodyAttrs, vnode.attrs.body), m(".modal-footer", vnode.attrs.footerAttrs, vnode.attrs.footer) ]) ]) ]) } } Modal.Dismiss = {view: vnode => { let tag = vnode.attrs.tag || "button[type=button].close" tag += "[data-dismiss=modal][aria-label=Close]" return m(tag, vnode.attrs.attrs, vnode.children) }} // Usage m(Modal, { headerAttrs: {title: "Modal title"}, header: [ m(Modal.Dismiss, [ m("span[aria-hidden=true]", m.trust("×")) ]) ], body: m("p", "Modal body text"), footer: [ m(Modal.Dismiss, {attrs: {class: "btn btn-secondary"}}, "Close"), m("button[type=button].btn.btn-primary", "Save changes") ] }) const Stats = { send(key, value) { // ... } } const MyModal = { view: () => m(Modal, { modalAttrs: { oncreate(vnode) { $(vnode.dom).on("show.bs.modal", () => { Stats.send("modal open", "MyModal") }) } }, // ... }) } m(Modal, { headerAttrs: {id: "my-modal-header"}, // ... }) ``` - This proposal ```js function Modal(vnode, old, update) { if (!vnode) return update(() => $(vnode.dom).modal("dispose")) if (!old) update(() => $(vnode.dom).modal(vnode.attrs.options)) if (!old || !!vnode.attrs.show !== !!old.attrs.show) { update(() => $(vnode.dom).modal(vnode.attrs.show ? "hide" : "show")) } return m(".modal[tabindex=-1][role=dialog]", vnode.attrs.modalAttrs, [ m(".modal-dialog[role=document]", { class: vnode.attrs.centered ? "modal-dialog-centered" : "" }, [ m(".modal-content", [ m(".modal-header", vnode.attrs.headerAttrs, [ m(".modal-title", vnode.attrs.titleAttrs, vnode.attrs.title), vnode.attrs.header ]), m(".modal-body", vnode.attrs.bodyAttrs, vnode.attrs.body), m(".modal-footer", vnode.attrs.footerAttrs, vnode.attrs.footer) ]) ]) ]) } Modal.Dismiss = ({ attrs: {tag = "button[type=button].close", attrs}, children }) => m(`${tag}[data-dismiss=modal][aria-label=Close]`, attrs, children) // Usage m(Modal, { headerAttrs: {title: "Modal title"}, header: [ m(Modal.Dismiss, [ m("span[aria-hidden=true]", m.trust("×")) ]) ], body: m("p", "Modal body text"), footer: [ m(Modal.Dismiss, {attrs: {class: "btn btn-secondary"}}, "Close"), m("button[type=button].btn.btn-primary", "Save changes") ] }) const Stats = { send(key, value) { // ... } } const MyModal = () => m(Modal, { modalAttrs: { // Just for show. Really, if you're doing this, you should be adding a // `onshow.bs.modal` event handler instead. render(vnode, old, update) { if (!old) { update(() => $(vnode.dom).on("show.bs.modal", () => { Stats.send("modal open", "MyModal") })) } } }, // ... }) // You can also finally attach IDs to your header itself m(Modal, { headerAttrs: {id: "my-modal-header"}, // ... }) ``` - For comparison, React's hooks API ```js // React hooks import {useEffect} from "react" function Modal(props) { const modal = useRef() const {ref: modalRef, ...modalProps} = props.modalProps useMutationEffect(() => { modalRef.current = modal.current }) useEffect(() => { $(modal.current).modal(props.options) return () => $(modal.current).modal("dispose") }) useEffect(() => { $(modal.current).modal(props.show ? "hide" : "show") }, [!!props.show]) return ( ) } Modal.Dismiss = ({tag: Tag = "button", ...props}) => ( ) // Usage } body={

Modal body text

} footer={<> Close } /> const Stats = { send(key, value) { // ... } } function MyModal() { const modal = useRef(modal) useEffect(() => { $(modal.current).on("show.bs.modal", () => { Stats.send("modal open", "MyModal") }) }) return } ``` If you'd like to see something a little more stateful to show the reducer side, here's what the `CounterButton` example from earlier looks like: ```js function CounterButton(vnode, old, update, state = {count: 0, updating: true}) { if (!vnode) return undefined if (state.updating) state.updating = false else if (vnode.attrs.color === old.attrs.color) return old return {next: state, view: [ m("button", { style: {color: vnode.attrs.color}, onclick: () => { state.updating = true; state.count++ } }, "Count: ", state.count) ]} } ```

barneycarroll commented 5 years ago

Returning the old vnode directly is nice - memoization for static components ends up simple to write and feels intuitive.

I see what you're saying about async effects invoked in views not matching up with the current implementation of update - they would happen in a new tick, while currently onupdate occurs synchronously immediately after views have persisted. Does it particularly matter, in practice? m.render is and will remain synchronous in its execution, but even now there is nothing we can do to stop authors introducing async code in any part of the application. If the contract is intentionally broken, I don't think it's of much concern to anyone.

It's neat to have onbeforeremove mirror initial iteration (now = undefined, then) v (now, then = undefined), but IMO this is completely off for the ergonomics of practical usage: as with the reducer proposal and the notion of passing hooks through the view function, it's simply way too much overloading. Making (now, then) available on all lifecycle hooks introduces greater consistency and enables convenient shorthand expressiveness without substantially increasing API surface or cognitive load of forked call graphs. Implementing #2098 enables me to practically achieve everything covered by the lifecycle (other than teardown) in a single function without any new APIs - the rest relies on substantial complication to execution logic for things we can already do with the the object tag. It doesn't fit the criteria of 'simplifying the component model' as the title suggests.

dead-claudia commented 5 years ago

@barneycarroll

I see what you're saying about async effects invoked in views not matching up with the current implementation of update - they would happen in a new tick, while currently onupdate occurs synchronously immediately after views have persisted. Does it particularly matter, in practice? m.render is and will remain synchronous in its execution, but even now there is nothing we can do to stop authors introducing async code in any part of the application. If the contract is intentionally broken, I don't think it's of much concern to anyone.

It's a concern for us because we need to block actual DOM removal (https://github.com/MithrilJS/mithril.js/issues/1881#issuecomment-440830870), but I agree it's not generally a concern elsewhere. I do also agree we don't really need scheduling in core, but it'd be nice to have something to direct users towards, even if it's not within render itself.

I see two main options here:

m.update(func) ↔ Promise.resolve().then(func): This would state that we do provide a vnode.dom property, but you have to schedule updates yourself, and m.render only returns after rendering what we were told to render. If you need to wait for users' updates, too, you could just do new Promise(m.update), which is resolved after all currently outstanding updates are executed. This would be 100% decoupled from m.render/m.redraw/m.mount, so users could easily use some other scheduling mechanism if they need to.
My update argument: This would state we support hooks, just you have to explicitly schedule them yourself. These would be run otherwise at the same time they would normally, so people would already know the timings and what to expect.

The first is actually incredibly simple to implement.

```js var scheduled = [] var promise = Promise.resolve() function report(e) { promise.then(function () { throw e }) } function execute() { for (var callback; (callback = scheduled.shift()) != null;) { try { callback() } catch (e) { report(e) } } } m.update = function (callback) { if (typeof callback !== "function") { throw new TypeError("`callback` must be a function!") } if (scheduled.length === 0) promise.then(execute) scheduled.push(func) } ``` This isn't just `Promise.resolve().then(func)` because on platforms with the polyfill (like IE), the order of `setTimeout` isn't guaranteed. On platforms with a native `Promise`, order is guaranteed, so we could just use that directly. Also, this is slightly faster than a full round-trip with the promise scheduler, since we're creating less garbage. I *seriously* wish there was a direct language-level means of scheduling microtasks, though...

It's neat to have onbeforeremove mirror initial iteration (now = undefined, then) v (now, then = undefined), but IMO this is completely off for the ergonomics of practical usage: as with the reducer proposal and the notion of passing hooks through the view function, it's simply way too much overloading.

What about going with the reducer, but changing it to (vnode, old, state) => view | {next, view, onremove} instead (using m.update above), where onremove is called without arguments? That would be just as simple to define and call.

dead-claudia commented 5 years ago

BTW, I'm starting to like the idea of (vnode, old, state) => view | {next, view, onremove} even better. I'll replace the original proposal with this, just with an extra update(next) for an easier time updating state and redrawing. (I've already found issues with not having it.)

dead-claudia commented 5 years ago

Just updated and rewrote the proposal entirely.

barneycarroll commented 5 years ago

I wish you could just post that as new stuff, the discourse and mutual reasoning becomes impossible to trace when posts are edited to such significant degrees

dead-claudia commented 5 years ago

@barneycarroll I understand. BTW, I did make it a point to do a couple things:

I included a "Previous proposal" (renamed "Initial proposal") section to hold the original, smaller-scoped proposal. I did this so it wouldn't get lost.
In all the issues under the umbrella of #2278 and #2284, I made it a point to keep a changelog of all edits, including even typo fixes. I don't go into great detail over them, but I do try to keep enough context that it doesn't get lost completely.

The reason why I redid the proposal and edited it instead of filing a new issue is because although it was substantially different, the previous proposal did almost nothing to solve the real issues with lifecycle hooks. It also didn't really match the spirit of its changes - it was simplifying it for us, but not for users. Saving 20-30 bytes by pulling a popular feature isn't doing anyone any real favors, especially since that requires migration and everything.

It was only after a bit of reflection after reading some of the responses here that I realized I was missing a major opportunity to actually fix things by really simplifying components (the title of the issue), not just for us but also for users. And of course, this realization meant I had to basically trash my proposal and start over. This issue was still the most appropriate place to discuss it, but the original proposal itself became mostly irrelevant.

I will make a counterpoint to not editing the main proposal: it's much easier to follow and understand what the proposal is if it's not spread across several comments in a conversation. It's also why Gitter is not a good place to have more complicated design discussions, and it's why many larger design-related proposals elsewhere end up with dedicated repos and everything. I've personally been considering opening up a new orphan branch on my personal fork to expand on #2278/#2284 and all its sub-issues, simply because of how broad and inter-connected it's become. (Once you find yourself needing intra-page hyperlinks, that's a good time to look into it.)

Now I really shouldn't be editing subsequent comments, even if they're just bug fixes.

dead-claudia commented 5 years ago

See this comment.

MithrilJS / mithril.js