Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: batch commit callbacks from all components in the render queue #4234

Merged
merged 8 commits into from
Jan 5, 2024
9 changes: 5 additions & 4 deletions compat/test/browser/useSyncExternalStore.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -658,10 +658,11 @@ describe('useSyncExternalStore', () => {
await act(() => {
store.set(1);
});
// Preact logs differ from React here cuz of how we do rerendering. We
// rerender subtrees and then commit effects so Child2 never sees the
// update to 1 cuz Child1 rerenders and runs its layout effects first.
assertLog([1, /*1,*/ 'Reset back to 0', 0, 0]);
// // Preact logs differ from React here cuz of how we do rerendering. We
// // rerender subtrees and then commit effects so Child2 never sees the
// // update to 1 cuz Child1 rerenders and runs its layout effects first.
// assertLog([1, /*1,*/ 'Reset back to 0', 0, 0]);
developit marked this conversation as resolved.
Show resolved Hide resolved
assertLog([1, 1, 'Reset back to 0', 0, 0]);
expect(container.textContent).to.equal('00');
});

Expand Down
30 changes: 22 additions & 8 deletions src/component.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { assign } from './util';
import { diff, commitRoot } from './diff/index';
import options from './options';
import { Fragment } from './create-element';
import { MODE_HYDRATE } from './constants';
import { EMPTY_ARR, MODE_HYDRATE } from './constants';

/**
* Base Component class. Provides `setState()` and `forceUpdate()`, which
Expand Down Expand Up @@ -120,12 +120,10 @@ export function getDomSibling(vnode, childIndex) {
* Trigger in-place re-rendering of a component.
* @param {Component} component The component to rerender
*/
function renderComponent(component) {
function renderComponent(component, commitQueue, refQueue) {
let oldVNode = component._vnode,
oldDom = oldVNode._dom,
parentDom = component._parentDom,
commitQueue = [],
refQueue = [];
parentDom = component._parentDom;

if (parentDom) {
const newVNode = assign({}, oldVNode);
Expand All @@ -146,11 +144,14 @@ function renderComponent(component) {
);

newVNode._parent._children[newVNode._index] = newVNode;
commitRoot(commitQueue, newVNode, refQueue);

newVNode._nextDom = undefined;

if (newVNode._dom != oldDom) {
updateParentDomPointers(newVNode);
}

return newVNode;
}
}

Expand Down Expand Up @@ -220,21 +221,34 @@ const depthSort = (a, b) => a._vnode._depth - b._vnode._depth;
/** Flush the render queue by rerendering all queued components */
function process() {
let c;
let commitQueue = [];
let refQueue = [];
let root;
rerenderQueue.sort(depthSort);
// Don't update `renderCount` yet. Keep its value non-zero to prevent unnecessary
// process() calls from getting scheduled while `queue` is still being consumed.
while ((c = rerenderQueue.shift())) {
if (c._dirty) {
let renderQueueLength = rerenderQueue.length;
renderComponent(c);
if (rerenderQueue.length > renderQueueLength) {
root = renderComponent(c, commitQueue, refQueue) || root;
// If this WAS the last component in the queue, run commit callbacks *before* we exit the tight loop.
// This is required in order for `componentDidMount(){this.setState()}` to be batched into one flush.
// Otherwise, also run commit callbacks if the render queue was mutated.
if (renderQueueLength === 0 || rerenderQueue.length > renderQueueLength) {
commitRoot(commitQueue, root, refQueue);
refQueue.length = commitQueue.length = 0;
root = undefined;
// When i.e. rerendering a provider additional new items can be injected, we want to
// keep the order from top to bottom with those new items so we can handle them in a
// single pass
rerenderQueue.sort(depthSort);
} else if (root) {
if (options._commit) options._commit(root, EMPTY_ARR);
}
}
}
if (root) commitRoot(commitQueue, root, refQueue);
// if (root) commitRoot(commitQueue, root, refQueue);
developit marked this conversation as resolved.
Show resolved Hide resolved
process._rerenderCount = 0;
}

Expand Down
2 changes: 0 additions & 2 deletions src/diff/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -311,8 +311,6 @@ export function diff(
* @param {VNode} root
*/
export function commitRoot(commitQueue, root, refQueue) {
root._nextDom = undefined;

for (let i = 0; i < refQueue.length; i++) {
applyRef(refQueue[i], refQueue[++i], refQueue[++i]);
}
Expand Down
3 changes: 3 additions & 0 deletions src/render.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ export function render(vnode, parentDom, replaceNode) {
);

// Flush all queued effects
// vnode._nextDom = undefined;
// if (options._commit) options._commit(vnode, EMPTY_ARR);
vnode._nextDom = undefined;
developit marked this conversation as resolved.
Show resolved Hide resolved
commitRoot(commitQueue, vnode, refQueue);
}

Expand Down
Loading