Mithril.js: Simplify our component model
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.statewould just be set tovnode.tag. - Closure components would be kept as-is.
- Class components would disappear in favor of closure components.
vnode.oninitwould disappear in favor of closure component constructors.- I'd rename
vnode.statetovnode._hooksso 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
oninitwith 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 function to this:
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
updateidentity - Add
m.bindm.changedhelper - 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 donew Comp(...)becausethis === Object.create(new Comp(...)), notthis === new Comp(...). They also get really verbose because in order to avoidthisissues, you're stuck withvnode.state.fooand similar, and destructuring isn't a real alternative when mutations get involved. - Class components have major
thisissues. 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
vnodeargument.
Also, every one of our lifecycle methods have issues:
oninitis 100% duplicated by class constructors and closure factories.onbeforeupdatecould literally be solved by a "don't redraw" sentinel + passing both parameters to the view. This also has other benefits: #2098.onbeforeremoveandonremoveare basically one and the same. You could get equivalent behavior by just calling the parentonremove, awaiting it, then calling the childonremoves depth-first without awaiting them.oncreateandonupdateare unnecessary boilerplate - we could just as easily switch to am.updatethat just schedules callbacks on the next tick.1, 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.requestto DOM event handlers), yet we still frequentlym.redraw. - For things like async loading, it's a waste to have
onremoveafter it's loaded. - Several of us can remember the big deal over
vnode.statebeing made no longer assignable in v2 for performance reasons. - I find myself needing the previous vnode just as often in
onupdateas I do inonbeforeupdate.
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
undefinedif 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 thevnode.stateto 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:2
oninitwould become just the first component call. The component would be rendered astag(vnode, undefined, undefined, update), and the first render can be detected by the previous vnode beingundefined.oncreatewould become invokingm.updatein the first component call.onbeforeupdatewould be conditionally returningoldfrom the component.viewwould be returning the vnode tree from either the top level or via theviewproperty.onupdatewould be invokingm.updateon subsequent component calls.onbeforeremovewould be optionally returning a promise fromonremove.onremovewould be returning a non-thenable fromonremove.
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 usingstateas necessary. - When this returns
true, it invokesstate.close()if such a method exists and signals to you that you should reinitialize it. - For convenience, when
state == null, this returnstrue.
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/onupdateare always scheduled after every call toview, 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 theviewdirectly - you'll save Mithril quite a bit of work.- The first render with v1/v2 is always
oninit>view> scheduleoncreate. 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> scheduleonupdate. 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.)
// 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 here3. 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.4
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.5
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.updatewould literally be this. It's pretty easy, and it's incredibly easy to teach. It's also simpler to implement than evenm.proporm.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
Mediacomponent, used in an example in https://github.com/MithrilJS/mithril.js/issues/2281. It's basically a stripped-down port ofreact-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'sviewwould literally be called asComp.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()inctrl.onunloadorctx.onunloadmeans 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.
isiahmeadows
All 25 comments
This also kind of relates to #2278.
isiahmeadows
on 14 Nov 2018
@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.
barneycarroll
on 14 Nov 2018
@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).
isiahmeadows
on 14 Nov 2018
Maybe we could add a deprecation warning in v2 when accessing vnode.state
StephanHoyer
on 14 Nov 2018
@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.stateuses. - 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)
}
}
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
}
nordfjord
on 14 Nov 2018
I love this idea, if indeed it doesn't have more wide-ranging repercussions
CreaturesInUnitards
on 14 Nov 2018
@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.)
isiahmeadows
on 14 Nov 2018
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.
spacejack
on 15 Nov 2018
@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.
CreaturesInUnitards
on 15 Nov 2018
@CreaturesInUnitards
I don't see the benefit to ripping
oninitout 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
oninitbecause you can't observeObject.create(object)calls fromobjectitself. - 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".
isiahmeadows
on 19 Nov 2018
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, andviewwould be fused intoview(vnode, old), whereold === undefinedon the first render.
- Return
oldto prevent subtree update. This works not unlike Mithril v0.2's old{subtree: "retain"}, and it'd replaceonbeforeupdate's functionality. - When you return
oldon first render (when it'sundefined), it's equivalent to an empty view, so on an empty tree, it's basically equivalent to not updating the subtree.
- Return
oncreate(vnode),onupdate(vnode), andonremove(vnode)would all fuse into a single sharedonupdate(vnode, old), whereold === undefinedon create andvnode === undefinedon remove.
- When
oldis notundefinedand is returned from theview, this hook is not fired after the update.
- When
onbeforeremove(vnode)would remain as-is.- If a
viewattribute exists on a vnode:
- The state is updated as normal.
- Existing children are cleared on the new vnode.
- The
viewis 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
viewfor their method name.)
- The lifecycle attrs on vnodes and components would become:
onupdate(vnode, old): voidview(vnode, old): vnodeonbeforeremove(vnode): any | Promise<any>
- Components would take two primary forms:
- Object components:
{view, onupdate?, onbeforeremove?} - Closure components:
() => {view, onupdate?, onbeforeremove?}
- Object components:
There are several benefits of this:
viewbeing callable from attrs with its result used would solve #2050.- Always receiving both the old and new attrs would solve #2098.
onupdatebeing 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.viewbeing 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
vnodeparameter 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.domandvnode.domSizeare uninitialized in theview, but you otherwise never receive a vnode with uninitialized data.
- If it's DOM-related, it goes in
- 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
configattribute 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.
- I think Mithril v0.2 was on to something by keeping it simple - its
Comparison
To draw from a couple of React's hooks API examples, here's how this would compare:
// 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:
// 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.)
isiahmeadows
on 19 Nov 2018
@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 viam(m.retain, attrs).
mwould literally ignore any and all children it has iftag === "!".
- 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()
},
}
@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.
isiahmeadows
on 21 Nov 2018
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
viewandonupdate.
@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
onbeforeremoveand just lettingonupdatereturn 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(myprev.children) seems really compelling... I'd prefer returningprevand checking that overprev.children, so it's a little more obvious and a little less ambiguous. If you really want a vnode tree, we could compare againstprev.instance, but that comes at the risk of exposing what's currently a private implementation detail. It also makes it impossible to attempt the nonsensicalreturn 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?
barneycarroll
on 22 Nov 2018
@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
onbeforeremoveexists. - Most things in
onremovecould be done while awaitingonbeforeremove- 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.
isiahmeadows
on 22 Nov 2018
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?
barneycarroll
on 22 Nov 2018
@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,
oldisundefined. - On final render,
vnodeisundefined. 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
oldwhenvnodeexists to prevent rendering any subtree.
- On first render,
- Elements would accept a single magic attribute
renderwhich 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:
// 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:
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
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
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
// 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 ( <div ref={modal} className="modal" tabIndex={-1} role="dialog" {...modalProps}> <div className={ `modal-dialog ${props.centered ? "modal-dialog-centered" : ""}` } role="document"> <div className="modal-content"> <div className="modal-header" {...props.headerProps}> <div className="modal-title" {...props.titleProps}>{props.title}</div> {props.header} </div> <div className="modal-body" {...props.bodyProps}>{props.body}</div> <div className="modal-footer" {...props.footerProps}>{props.footer}</div> </div> </div> </div> ) } Modal.Dismiss = ({tag: Tag = "button", ...props}) => ( <Tag className="close" type="button" data-dismiss="modal" aria-label="Close" {...props} /> ) // Usage <Modal headerProps={{title: "Modal title"}} header={<Modal.Dismiss> <span aria-hidden="true" dangerouslySetInnerHTML="×" /> </Modal.Dismiss>} body={<p>Modal body text</p>} footer={<> <Modal.Dismiss attrs={{className: "btn btn-secondary"}}>Close</Modal.Dismiss> <button type="button" className="btn btn-primary">Save changes</button> </>} /> const Stats = { send(key, value) { // ... } } function MyModal() { const modal = useRef(modal) useEffect(() => { $(modal.current).on("show.bs.modal", () => { Stats.send("modal open", "MyModal") }) }) return <Modal modalProps={{ref: modal}} // ... /> } <Modal headerProps={{id: "my-modal-header"}} // ... />
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:
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)
]}
}
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.
barneycarroll
on 22 Nov 2018
@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 avnode.domproperty, but you have to schedule updates yourself, andm.renderonly returns after rendering what we were told to render. If you need to wait for users' updates, too, you could just donew Promise(m.update), which is resolved after all currently outstanding updates are executed. This would be 100% decoupled fromm.render/m.redraw/m.mount, so users could easily use some other scheduling mechanism if they need to.- My
updateargument: 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.
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.
isiahmeadows
on 22 Nov 2018
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.)
isiahmeadows
on 26 Nov 2018
Just updated and rewrote the proposal entirely.
isiahmeadows
on 26 Nov 2018
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
barneycarroll
on 26 Nov 2018
@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.
isiahmeadows
on 27 Nov 2018
isiahmeadows
on 27 Nov 2018
Related issues
dhinesh03
路
4Comments
barneycarroll
路
3Comments
tivac
路
3Comments
omenking
路
3Comments
isiahmeadows
路
4Comments
Most helpful comment
I wonder whether we could add a third type of component
Vnode -> () -> VnodeThat is to say if a closure component returns a function, it's treated as a view.
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
initComponentmethod