Separate lifecycle methods from attributes and components #2918
Replies: 12 comments
-
Yet another return of ancient APIs of pre 1.0 times, nice! |
Beta Was this translation helpful? Give feedback.
-
I really like the idea, I've been exploring custom vnode types in my code bases to support first class view reactivity and I think its a great direction. So I'm 👍 I also really dislike Is this the right issue to bikeshed names? I'm thinking I can imagine a newcomer internalizing that more easily. When they see
But I'd prefer But other than that I think it's great! |
Beta Was this translation helpful? Give feedback.
-
return m("div.bs-modal",
m.access((dom, isInit) => {
if (isInit) {
$(dom).modal()
}
if (vnode.attrs.show) {
$(dom).modal("show")
} else {
$(dom).modal("hide")
}
}),
m.release(dom => {
$(dom).modal("destroy")
})
) @isiahmeadows in this context, where is the |
Beta Was this translation helpful? Give feedback.
-
@CreaturesInUnitards It's in a hypothetical component - I just left it out for brevity (and I didn't feel like spending too much time figuring out API structure and such). If we went with #2690, it'd use |
Beta Was this translation helpful? Give feedback.
-
@isiahmeadows I get that, but you're proposing a major, breaking change — "API structure and such" are everything. The merits of this proposal are almost entirely contingent on the API, specifically what's in scope OOTB. So if, for example, the thought was #2690 and the ref is |
Beta Was this translation helpful? Give feedback.
-
@CreaturesInUnitards I get that - note the tags. 😉 I'm using separate issues as these proposals are technically separable in a sense, even if that's not my ultimate intent. I'm also trying to keep it accessible to those not actively following the discussion. |
Beta Was this translation helpful? Give feedback.
-
@isiahmeadows I didn't mean to suggest you didn't know these are breaking changes 😀 |
Beta Was this translation helpful? Give feedback.
-
Okay, redid the proposal to sit a little closer to a happy middle, based on discussion both in Gitter and here in various issues. You can read the updated description. TL;DR: I'm basically locking all the lifecycle magic down to I also corrected a bug where I misremembered the relevant Bootstrap modal method name. In Bootstrap v5, it'd look more like this: // With #2690
function Comp(ctx) {
let modal
return () => m.fragment({
afterRender([dom]) {
if (modal == null) {
modal = new bootstrap.Modal(dom, {keyboard: false})
if (ctx.attrs.show) modal.show()
} else {
if (ctx.attrs.show) {
modal.show()
} else {
modal.hide()
}
modal.handleUpdate()
}
},
beforeRemove([dom]) {
if (!attrs.show) return
modal.handleUpdate()
modal.hide()
return new Promise(resolve => {
dom.addEventListener("hidden.bs.modal", resolve, {once: true})
})
},
afterRemove([dom]) {
modal.dispose()
},
}, m("div.bs-modal", ctx.attrs.children))
}
// With current component API
function Comp() {
let modal
return {
view: ({attrs, children}) => m.fragment({
afterRender([dom]) {
if (modal == null) {
modal = new bootstrap.Modal(dom, {keyboard: false})
if (attrs.show) modal.show()
} else {
if (attrs.show) {
modal.show()
} else {
modal.hide()
}
modal.handleUpdate()
}
},
beforeRemove([dom]) {
if (!attrs.show) return
modal.handleUpdate()
modal.hide()
return new Promise(resolve => {
dom.addEventListener("hidden.bs.modal", resolve, {once: true})
})
},
afterRemove() {
modal.dispose()
},
}, m("div.bs-modal", children))
}
} |
Beta Was this translation helpful? Give feedback.
-
@barneycarroll Updated it with the old proposal's text. Took a little bit of hackery, but it got done. |
Beta Was this translation helpful? Give feedback.
-
Update: this should be a new primitive vnode with its own dedicated tag and name, to keep it out of the hot path of updates, as fragments are extremely common. The shape of the proposal still remains, so I'm not going to update names of everything (yet). |
Beta Was this translation helpful? Give feedback.
-
@dead-claudia given that children are normalized to arrays, and that we want hooks to support fragments, how would the representation and handling of Edit: you'd use it as siblings maybe? |
Beta Was this translation helpful? Give feedback.
-
@pygy I'll start off by answering this question, even though it's arguably moderately pointless with what I'm about to follow it up with.
I left that intentionally unspecified, but if they're designed to work as fragments, then yes, that's the intent. The sibling idea is one I've played around with for quite a while (years, really). They'd exist as siblings, executed/scheduled whenever visited, and they would receive whatever element is their nearest parent. Of course, this is a bit radical in the world of virtual DOM, but that of course could go without stating. I was torn on how much I liked it, because there's definitely some strong benefits, yet cons that get as large as you logically take it.
|
Beta Was this translation helpful? Give feedback.
-
(Related: #2688)
Old proposal
### Description Replace these idioms:With this idiom:
m.access(...)
would have a tag of"+"
, andm.release(...)
would have a tag of"-"
.Why
It's simpler and more flexible, and when combined with #2688, you also get the ability to diff and patch based on attributes for free, which isn't possible with the current framework.
Oh, and it will also provide a few perf benefits - this blog post can give you an idea why for part of it, but also not accessing dynamic properties that may or may not exist can also help a lot.
I suspect a 5-10% perf increase and a significant library size decrease out of this.
Possible Implementation
createNode
andupdateNode
, ifvnode.tag === "+"
, schedule the callback withparent
andisInit
set accordingly (true
increateNode
,false
inupdateNode
) and treat it otherwise as equivalent toundefined
.createNode
andupdateNode
, ifvnode.tag === "-"
, treat it as equivalent toundefined
.removeNode
, ignorevnode.tag === "+"
and invoke the callback ifvnode.tag === "-"
.vnode.dom
withvnode.state
in the vnode object, and use it for both element references, component state, and access/remove callbacks.Mithril version:
Platform and OS:
Project:
Is this something you're interested in implementing yourself? Yes
Description
Replace these idioms:
With this idiom:
m.fragment(...)
would have the same tag it normally does. This would also by side effect meanm.censor
just becomesm.censor = ({key, ...rest}) => rest
absent user-provided keys, though we could just as easily stripkey
internally like React does (the smart thing to do IMHO) and not need it anymore.The parameter of each is actually an array of DOM nodes. And while it's technically less efficient, it's likely to be minor in practice as significant DOM work is rare, and we're talking small numbers compared to a significantly more involved algorithm plus the native browser's own updating mechanisms plus all the adapting logic between JS and native just to invoke the browser APIs - a single array allocation is nothing compared to that, just slightly more GC churn (as it's retained for a decent amount of time).
I'm leaving out
onbeforeupdate
from this proposal as that's covered by #2688 and is being discussed separately.Why
It's simpler and more flexible, and when combined with #2688, you also get the ability to diff and patch based on attributes for free, which isn't possible with the current framework.
Oh, and it will also provide a few perf benefits:
I suspect a 5-10% perf increase and a mild library size decrease out of this, based on my experience with Mithril and its performance profile.
Possible Implementation
createComponent
and these lines fromupdateComponent
.updateLifecycle
to instead schedulesource.afterRender
with an array ofvnode.domSize
nodes starting fromvnode.dom
and continuing throughelem.nextSibling
.onremove
to instead invokevnode.attrs.afterRemove
and only ifvnode.tag === "[" && vnode.attrs != null && typeof vnode.attrs.afterRemove === "function"
.This would also make it such that
vnode.state
andvnode.dom
are mutually exclusive, so we could merge those accordingly.Open Questions
Beta Was this translation helpful? Give feedback.
All reactions