Re-purpose renderCallbacks as a general per-component commit ph…#2011
Conversation
| let mounts = []; | ||
| let newDom = diff(parentDom, vnode, assign({}, vnode), component._context, parentDom.ownerSVGElement!==undefined, null, mounts, oldDom == null ? getDomSibling(vnode) : oldDom); | ||
| commitRoot(mounts, vnode); | ||
| let commitQueue = []; |
There was a problem hiding this comment.
Do people like this rename of mounts to commitQueue? Since the mounts array isn't just used for componentDidMount anymore I figured it probably needed a new name that better communicated that.
There was a problem hiding this comment.
+1 for the name change. It better reflects the actual thing it's used for 👍
| oldState = c.state; | ||
| if (c.componentDidUpdate!=null) { | ||
| c._renderCallbacks.push(() => { | ||
| c.componentDidUpdate(oldProps, oldState, snapshot); |
There was a problem hiding this comment.
Note, I've moved the queuing of cDU up here to reuse the isNew check and to also no longer need to check oldProps == null as a way to determine if sCU returned false. Plus it lives with the rest of the lifecycle methods so I think reasoning about lifecycle methods will be a little easier.
| c.componentDidMount(); | ||
| commitQueue = c._renderCallbacks; | ||
| c._renderCallbacks = []; | ||
| commitQueue.some(cb => { cb.call(c); }); |
There was a problem hiding this comment.
@developit, it seems back in #648, you expressed perf concerns about a general callback queue, which I think is what I've implemented here. Any concerns with this implementation?
| this.state = { i: 0 }; | ||
| } | ||
|
|
||
| componentDidMount() { |
There was a problem hiding this comment.
Syncrhonously calling rerender inside of cDM doesn't effectively replicate how Preact's diffing algorithm works (we batch updates and diff them later). Plus it causes an infinite loop so I rewrote this test.
| render(<Foo moved={false} />, scratch); | ||
|
|
||
| expect(ops).to.deep.equal(['Mount Stateful2', 'Mount Stateful1']); | ||
| expect(ops).to.deep.equal(['Mount Stateful1', 'Mount Stateful2']); |
There was a problem hiding this comment.
Previously Preact called cDM for siblings in reverse order. Now we call cDM for siblings from top to bottom. Since cDM is invoked after all DOM operations are done, I don't think this order matters as much, but for what its worth, our order matches React now (codesandbox).
This sibling re-ordering also impacted the should handle hoisted component vnodes without DOM test.
marvinhagemeister
left a comment
There was a problem hiding this comment.
Hero 🙌💯 I have a feeling like this PR will resolve even more compatibility issues than we're aware of. Iirc a few users running into the ordering issue when it comes to lifecycles. Super excited about landing this PR👍🍀
JoviDeCroock
left a comment
There was a problem hiding this comment.
Very excited about this, concern: componentDidUpdate will now fire when a component is unmounted + props updated, no?
|
Thanks! 😄 @JoviDeCroock I think |
fe8527d to
83a03e3
Compare
Currently the only lifecycle method that is invoked after all work for a diff is done is
componentDidMount. This PR re-purposes the_renderCallbacksqueue in each component to be a general "after work" or "commit phase" callback queue. I use this queue to queue up several kinds of methods:I believe all of these methods are intended to be invoked after a diff is completed and all dom work is done. This PR makes that true! And the best part, only for 11B!
I'm hoping that we can further reuse this queue for the
useLayoutEffecthook since this hook is basically just a way to directly "hook" into this mechanism (running a callback synchronously after a diff is complete), but that'll come in another PR.I included test from #884 (which previously tried to add this behavior for cDU) and fixed the test from #506 to verify the changes.
Fixes #648
Fixes #1223