[FIX] components: solve missing update (concurrency issue)

Before this commit, in some specific situations, owl could skip updating
the DOM, even though it should. Here is what could happen:

- we have a parent component (A) and a child component (B)
- B depends on some reactive props from A
- we update some state that results in A being rendered, and B updated
- before this render is applied to the dom, we update some state again,
which causes A to be rendered again
- however, this new render is such that the B props are now identical,
so it will skip rendering B
- it will then apply the result of the render to the DOM => only A is
updated but B should also be updated!

The problem comes from the fact that Owl committed the props to the
component right after the rendering, to the new props were used in the
props comparison method to decide if B should be updated. This is
incorrect, we should always use the current props to decide what to do,
and only commit them to the component after it has been patched.
This commit is contained in:
Géry Debongnie
2023-04-17 10:12:16 +02:00
committed by Sam Degueldre
parent 5d40cc112d
commit 9a5ff7a619
4 changed files with 144 additions and 2 deletions
+3 -2
View File
@@ -73,6 +73,7 @@ export class ComponentNode<P extends Props = any, E = any> implements VNode<Comp
forceNextRender: boolean = false;
parentKey: string | null;
props: P;
nextProps: P | null = null;
renderFn: Function;
parent: ComponentNode | null;
@@ -220,7 +221,7 @@ export class ComponentNode<P extends Props = any, E = any> implements VNode<Comp
}
async updateAndRender(props: P, parentFiber: Fiber) {
const rawProps = props;
this.nextProps = props;
props = Object.assign({}, props);
// update
const fiber = makeChildFiber(this, parentFiber);
@@ -245,7 +246,6 @@ export class ComponentNode<P extends Props = any, E = any> implements VNode<Comp
return;
}
component.props = props;
this.props = rawProps;
fiber.render();
const parentRoot = parentFiber.root!;
if (this.willPatch.length) {
@@ -327,6 +327,7 @@ export class ComponentNode<P extends Props = any, E = any> implements VNode<Comp
// by the component will be patched independently in the appropriate
// fiber.complete
this._patch();
this.props = this.nextProps!;
}
}
_patch() {
@@ -1252,6 +1252,31 @@ exports[`delayed render does not go through when t-component value changed 3`] =
}"
`;
exports[`delayed render is not cancelled by upcoming render 1`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
const comp1 = app.createComponent(\`B\`, true, false, false, [\\"state\\",\\"isEmpty\\"]);
return function template(ctx, node, key = \\"\\") {
return comp1({state: ctx['state'],isEmpty: ctx['state'].groups.length===0}, key + \`__1\`, node, this, null);
}
}"
`;
exports[`delayed render is not cancelled by upcoming render 2`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
return function template(ctx, node, key = \\"\\") {
const b2 = text(ctx['props'].state.groups.length);
const b3 = text(ctx['props'].state.config.test);
return multi([b2, b3]);
}
}"
`;
exports[`delayed rendering, but then initial rendering is cancelled by yet another render 1`] = `
"function anonymous(app, bdom, helpers
) {
+74
View File
@@ -4126,6 +4126,80 @@ test("delayed render does not go through when t-component value changed", async
]).toBeLogged();
});
test("delayed render is not cancelled by upcoming render", async () => {
let b: any;
class B extends Component {
static template = xml`
<t t-esc="props.state.groups.length"/>
<t t-esc="props.state.config.test"/>`;
setup() {
useLogLifecycle();
b = this;
}
}
class A extends Component {
static components = { B };
static template = xml`<B state="state" isEmpty="state.groups.length === 0"/>`;
state = useState({ groups: [], config: { test: "initial" } });
setup() {
useLogLifecycle();
}
}
await mount(A, fixture);
expect([
"A:setup",
"A:willStart",
"A:willRender",
"B:setup",
"B:willStart",
"A:rendered",
"B:willRender",
"B:rendered",
"B:mounted",
"A:mounted",
]).toBeLogged();
expect(fixture.innerHTML).toBe("0initial");
b.props.state.config.test = "red";
b.props.state.groups.push(1);
expect([]).toBeLogged();
await Promise.resolve();
await Promise.resolve();
await Promise.resolve();
await Promise.resolve();
await Promise.resolve();
b.props.state.config.test = "black";
b.props.state.groups.push(1);
expect([
"A:willRender",
"B:willUpdateProps",
"A:rendered",
"B:willRender",
"B:rendered",
]).toBeLogged();
await nextTick();
expect(fixture.innerHTML).toBe("2black");
expect([
"A:willRender",
"B:willUpdateProps",
"A:rendered",
"B:willRender",
"B:rendered",
"A:willPatch",
"B:willPatch",
"B:patched",
"A:patched",
]).toBeLogged();
});
// test.skip("components with shouldUpdate=false", async () => {
// const state = { p: 1, cc: 10 };
@@ -0,0 +1,42 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP
exports[`shadow_dom can bind event handler 1`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
let block1 = createBlock(\`<button block-handler-0=\\"click\\">Click</button>\`);
return function template(ctx, node, key = \\"\\") {
let hdlr1 = [ctx['add'], ctx];
return block1([hdlr1]);
}
}"
`;
exports[`shadow_dom can mount app 1`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
let block1 = createBlock(\`<div class=\\"my-div\\"/>\`);
return function template(ctx, node, key = \\"\\") {
return block1();
}
}"
`;
exports[`shadow_dom useRef hook 1`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
let block1 = createBlock(\`<div class=\\"my-div\\" block-ref=\\"0\\"/>\`);
return function template(ctx, node, key = \\"\\") {
let ref1 = (el) => this.__owl__.setRef((\`refName\`), el);
return block1([ref1]);
}
}"
`;