Skip to content

Commit

Permalink
[FIX] compiler: t-key and t-ref together
Browse files Browse the repository at this point in the history 
Have a t-ref on a DOM node with a t-key.
Change the t-key.

Before this commit, the old block removed its element from the component's refs *after*
the new block had been mounted, meaning that in effect, the resulting
ref at the end of the whole patch was null.

After this commit, we only remove an element from the component's ref if that very same
element was indeed the ref. (otherwise it means someone else has changed the ref.)
  • Loading branch information
@kebeclibre @ged-odoo
kebeclibre authored and ged-odoo committed Feb 20, 2023
1 parent 1291f1f commit 0717fe2
Show file tree
Hide file tree
Showing 9 changed files with 117 additions and 32 deletions.
6 changes: 4 additions & 2 deletions src/compiler/code_generator.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -672,8 +672,9 @@ export class CodeGenerator {
this.target.hasRef = true;
const isDynamic = INTERP_REGEXP.test(ast.ref);
if (isDynamic) {
this.helpers.add("singleRefSetter");
const str = replaceDynamicParts(ast.ref, (expr) => this.captureExpression(expr, true));
const idx = block!.insertData(`(el) => refs[${str}] = el`, "ref");
const idx = block!.insertData(`singleRefSetter(refs, ${str})`, "ref");
attrs["block-ref"] = String(idx);
} else {
let name = ast.ref;
Expand All @@ -686,7 +687,8 @@ export class CodeGenerator {
info[1] = `multiRefSetter(refs, \`${name}\`)`;
} else {
let id = generateId("ref");
this.target.refInfo[name] = [id, `(el) => refs[\`${name}\`] = el`];
this.helpers.add("singleRefSetter");
this.target.refInfo[name] = [id, `singleRefSetter(refs, \`${name}\`)`];
const index = block!.data.push(id) - 1;
attrs["block-ref"] = String(index);
}
Expand Down
11 changes: 11 additions & 0 deletions src/runtime/template_helpers.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,16 @@ function multiRefSetter(refs: RefMap, name: string): RefSetter {
};
}

function singleRefSetter(refs: RefMap, name: string): RefSetter {
let _el: HTMLElement | null = null;
return (el) => {
if (el || refs[name] === _el) {
refs[name] = el;
_el = el;
}
};
}

/**
* Validate the component props (or next props) against the (static) props
* description. This is potentially an expensive operation: it may needs to
Expand Down Expand Up @@ -260,6 +270,7 @@ export const helpers = {
prepareList,
setContextValue,
multiRefSetter,
singleRefSetter,
shallowEqual,
toNumber,
validateProps,
Expand Down
6 changes: 3 additions & 3 deletions tests/compiler/__snapshots__/misc.test.ts.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ exports[`misc other complex template 1`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
let { prepareList, withKey } = helpers;
let { prepareList, withKey, singleRefSetter } = helpers;
const callTemplate_1 = app.getTemplate(\`LOAD_INFOS_TEMPLATE\`);
const comp1 = app.createComponent(\`BundlesList\`, true, false, false, false);
const comp2 = app.createComponent(\`BundlesList\`, true, false, false, false);
Expand Down Expand Up @@ -218,8 +218,8 @@ exports[`misc other complex template 1`] = `
return function template(ctx, node, key = \\"\\") {
const refs = this.__owl__.refs;
const ref1 = (el) => refs[\`search_input\`] = el;
const ref2 = (el) => refs[\`settings_menu\`] = el;
const ref1 = singleRefSetter(refs, \`search_input\`);
const ref2 = singleRefSetter(refs, \`settings_menu\`);
let b2,b4,b14,b17,b22,b23,b24,b25;
let attr1 = \`/runbot/\${ctx['project'].slug}\`;
let txt1 = ctx['project'].name;
Expand Down
24 changes: 15 additions & 9 deletions tests/compiler/__snapshots__/t_ref.test.ts.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,14 @@ exports[`t-ref can get a dynamic ref on a node 1`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
let { singleRefSetter } = helpers;
let block1 = createBlock(\`<div><span block-ref=\\"0\\"/></div>\`);
return function template(ctx, node, key = \\"\\") {
const refs = this.__owl__.refs;
const v1 = ctx['id'];
let ref1 = (el) => refs[\`myspan\${v1}\`] = el;
let ref1 = singleRefSetter(refs, \`myspan\${v1}\`);
return block1([ref1]);
}
}"
Expand All @@ -20,13 +21,14 @@ exports[`t-ref can get a dynamic ref on a node, alternate syntax 1`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
let { singleRefSetter } = helpers;
let block1 = createBlock(\`<div><span block-ref=\\"0\\"/></div>\`);
return function template(ctx, node, key = \\"\\") {
const refs = this.__owl__.refs;
const v1 = ctx['id'];
let ref1 = (el) => refs[\`myspan\${v1}\`] = el;
let ref1 = singleRefSetter(refs, \`myspan\${v1}\`);
return block1([ref1]);
}
}"
Expand All @@ -36,12 +38,13 @@ exports[`t-ref can get a ref on a node 1`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
let { singleRefSetter } = helpers;
let block1 = createBlock(\`<div><span block-ref=\\"0\\"/></div>\`);
return function template(ctx, node, key = \\"\\") {
const refs = this.__owl__.refs;
const ref1 = (el) => refs[\`myspan\`] = el;
const ref1 = singleRefSetter(refs, \`myspan\`);
return block1([ref1]);
}
}"
Expand All @@ -66,12 +69,13 @@ exports[`t-ref ref in a t-call 2`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
let { singleRefSetter } = helpers;
let block1 = createBlock(\`<div>1<span block-ref=\\"0\\"/>2</div>\`);
return function template(ctx, node, key = \\"\\") {
const refs = this.__owl__.refs;
const ref1 = (el) => refs[\`name\`] = el;
const ref1 = singleRefSetter(refs, \`name\`);
return block1([ref1]);
}
}"
Expand All @@ -81,13 +85,14 @@ exports[`t-ref ref in a t-if 1`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
let { singleRefSetter } = helpers;
let block1 = createBlock(\`<div><block-child-0/></div>\`);
let block2 = createBlock(\`<span block-ref=\\"0\\"/>\`);
return function template(ctx, node, key = \\"\\") {
const refs = this.__owl__.refs;
const ref1 = (el) => refs[\`name\`] = el;
const ref1 = singleRefSetter(refs, \`name\`);
let b2;
if (ctx['condition']) {
b2 = block2([ref1]);
Expand All @@ -101,7 +106,7 @@ exports[`t-ref refs in a loop 1`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
let { prepareList, withKey } = helpers;
let { prepareList, singleRefSetter, withKey } = helpers;
let block1 = createBlock(\`<div><block-child-0/></div>\`);
let block3 = createBlock(\`<div block-ref=\\"0\\"><block-text-1/></div>\`);
Expand All @@ -115,7 +120,7 @@ exports[`t-ref refs in a loop 1`] = `
const key1 = ctx['item'];
const tKey_1 = ctx['item'];
const v1 = ctx['item'];
let ref1 = (el) => refs[(v1)] = el;
let ref1 = singleRefSetter(refs, (v1));
let txt1 = ctx['item'];
c_block2[i1] = withKey(block3([ref1, txt1]), tKey_1 + key1);
}
Expand All @@ -129,14 +134,15 @@ exports[`t-ref two refs, one in a t-if 1`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
let { singleRefSetter } = helpers;
let block1 = createBlock(\`<div><block-child-0/><p block-ref=\\"0\\"/></div>\`);
let block2 = createBlock(\`<span block-ref=\\"0\\"/>\`);
return function template(ctx, node, key = \\"\\") {
const refs = this.__owl__.refs;
const ref1 = (el) => refs[\`name\`] = el;
const ref2 = (el) => refs[\`p\`] = el;
const ref1 = singleRefSetter(refs, \`name\`);
const ref2 = singleRefSetter(refs, \`p\`);
let b2;
if (ctx['condition']) {
b2 = block2([ref1]);
Expand Down
16 changes: 10 additions & 6 deletions tests/components/__snapshots__/hooks.test.ts.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,15 @@ exports[`hooks autofocus hook input in a t-if 1`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
let { singleRefSetter } = helpers;
let block1 = createBlock(\`<div><input block-ref=\\"0\\"/><block-child-0/></div>\`);
let block2 = createBlock(\`<input block-ref=\\"0\\"/>\`);
return function template(ctx, node, key = \\"\\") {
const refs = this.__owl__.refs;
const ref1 = (el) => refs[\`input1\`] = el;
const ref2 = (el) => refs[\`input2\`] = el;
const ref1 = singleRefSetter(refs, \`input1\`);
const ref2 = singleRefSetter(refs, \`input2\`);
let b2;
if (ctx['state'].flag) {
b2 = block2([ref2]);
Expand All @@ -25,13 +26,14 @@ exports[`hooks autofocus hook simple input 1`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
let { singleRefSetter } = helpers;
let block1 = createBlock(\`<div><input block-ref=\\"0\\"/><input block-ref=\\"1\\"/></div>\`);
return function template(ctx, node, key = \\"\\") {
const refs = this.__owl__.refs;
const ref1 = (el) => refs[\`input1\`] = el;
const ref2 = (el) => refs[\`input2\`] = el;
const ref1 = singleRefSetter(refs, \`input1\`);
const ref2 = singleRefSetter(refs, \`input2\`);
return block1([ref1, ref2]);
}
}"
Expand Down Expand Up @@ -264,12 +266,13 @@ exports[`hooks useEffect hook effect can depend on stuff in dom 1`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
let { singleRefSetter } = helpers;
let block2 = createBlock(\`<div block-ref=\\"0\\"/>\`);
return function template(ctx, node, key = \\"\\") {
const refs = this.__owl__.refs;
const ref1 = (el) => refs[\`div\`] = el;
const ref1 = singleRefSetter(refs, \`div\`);
let b2;
if (ctx['state'].value) {
b2 = block2([ref1]);
Expand Down Expand Up @@ -353,12 +356,13 @@ exports[`hooks useRef hook: basic use 1`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
let { singleRefSetter } = helpers;
let block1 = createBlock(\`<div><button block-ref=\\"0\\"><block-text-1/></button></div>\`);
return function template(ctx, node, key = \\"\\") {
const refs = this.__owl__.refs;
const ref1 = (el) => refs[\`button\`] = el;
const ref1 = singleRefSetter(refs, \`button\`);
let txt1 = ctx['value'];
return block1([ref1, txt1]);
}
Expand Down
36 changes: 30 additions & 6 deletions tests/components/__snapshots__/refs.test.ts.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,13 @@ exports[`refs basic use 1`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
let { singleRefSetter } = helpers;
let block1 = createBlock(\`<div block-ref=\\"0\\"/>\`);
return function template(ctx, node, key = \\"\\") {
const refs = this.__owl__.refs;
const ref1 = (el) => refs[\`div\`] = el;
const ref1 = singleRefSetter(refs, \`div\`);
return block1([ref1]);
}
}"
Expand All @@ -19,7 +20,7 @@ exports[`refs can use 2 refs with same name in a t-if/t-else situation 1`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
let { multiRefSetter } = helpers;
let { singleRefSetter, multiRefSetter } = helpers;
let block2 = createBlock(\`<div block-ref=\\"0\\"/>\`);
let block3 = createBlock(\`<span block-ref=\\"0\\"/>\`);
Expand All @@ -42,13 +43,14 @@ exports[`refs refs and recursive templates 1`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
let { singleRefSetter } = helpers;
const comp1 = app.createComponent(\`Test\`, true, false, false, false);
let block1 = createBlock(\`<p block-ref=\\"0\\"><block-text-1/><block-child-0/></p>\`);
return function template(ctx, node, key = \\"\\") {
const refs = this.__owl__.refs;
const ref1 = (el) => refs[\`root\`] = el;
const ref1 = singleRefSetter(refs, \`root\`);
let b2;
let txt1 = ctx['props'].tree.value;
if (ctx['props'].tree.child) {
Expand All @@ -59,19 +61,41 @@ exports[`refs refs and recursive templates 1`] = `
}"
`;
exports[`refs refs and t-key 1`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
let { singleRefSetter } = helpers;
let block2 = createBlock(\`<button block-handler-0=\\"click\\"/>\`);
let block3 = createBlock(\`<p block-ref=\\"0\\"/>\`);
return function template(ctx, node, key = \\"\\") {
const refs = this.__owl__.refs;
const ref1 = singleRefSetter(refs, \`root\`);
const v1 = ctx['state'];
let hdlr1 = [()=>v1.renderId++, ctx];
const b2 = block2([hdlr1]);
const tKey_1 = ctx['state'].renderId;
const b3 = toggler(tKey_1, block3([ref1]));
return multi([b2, b3]);
}
}"
`;
exports[`refs refs are properly bound in slots 1`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
let { capture, markRaw } = helpers;
let { capture, singleRefSetter, markRaw } = helpers;
const comp1 = app.createComponent(\`Dialog\`, true, true, false, true);
let block1 = createBlock(\`<div><span class=\\"counter\\"><block-text-0/></span><block-child-0/></div>\`);
let block2 = createBlock(\`<button block-handler-0=\\"click\\" block-ref=\\"1\\">do something</button>\`);
function slot1(ctx, node, key = \\"\\") {
const refs = this.__owl__.refs;
const ref1 = (el) => refs[\`myButton\`] = el;
const ref1 = singleRefSetter(refs, \`myButton\`);
let hdlr1 = [ctx['doSomething'], ctx];
return block2([hdlr1, ref1]);
}
Expand Down Expand Up @@ -104,7 +128,7 @@ exports[`refs throws if there are 2 same refs at the same time 1`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
let { multiRefSetter } = helpers;
let { singleRefSetter, multiRefSetter } = helpers;
let block2 = createBlock(\`<div block-ref=\\"0\\"/>\`);
let block3 = createBlock(\`<span block-ref=\\"0\\"/>\`);
Expand Down
4 changes: 2 additions & 2 deletions tests/components/__snapshots__/slots.test.ts.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,15 +158,15 @@ exports[`slots can render node with t-ref and Component in same slot 1`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
let { markRaw } = helpers;
let { singleRefSetter, markRaw } = helpers;
const comp1 = app.createComponent(\`Child\`, true, false, false, true);
const comp2 = app.createComponent(\`Child\`, true, true, false, true);

let block2 = createBlock(\`<div block-ref=\\"0\\"/>\`);

function slot1(ctx, node, key = \\"\\") {
const refs = this.__owl__.refs;
const ref1 = (el) => refs[\`div\`] = el;
const ref1 = singleRefSetter(refs, \`div\`);
const b2 = block2([ref1]);
const b3 = comp1({}, key + \`__1\`, node, this, null);
return multi([b2, b3]);
Expand Down
6 changes: 3 additions & 3 deletions tests/components/__snapshots__/t_call.test.ts.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,7 @@ exports[`t-call t-call-context: ComponentNode is not looked up in the context 2`
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
let { bind, capture, isBoundary, withDefault, setContextValue, markRaw } = helpers;
let { singleRefSetter, bind, capture, isBoundary, withDefault, setContextValue, markRaw } = helpers;
const comp1 = app.createComponent(\`Child\`, true, true, false, false);
let block2 = createBlock(\`<div block-ref=\\"0\\">outside slot</div>\`);
Expand All @@ -543,7 +543,7 @@ exports[`t-call t-call-context: ComponentNode is not looked up in the context 2`
function slot1(ctx, node, key = \\"\\") {
const refs = this.__owl__.refs;
const ref2 = (el) => refs[\`myRef2\`] = el;
const ref2 = singleRefSetter(refs, \`myRef2\`);
ctx = Object.create(ctx);
ctx[isBoundary] = 1
const b4 = block4([ref2]);
Expand All @@ -555,7 +555,7 @@ exports[`t-call t-call-context: ComponentNode is not looked up in the context 2`
return function template(ctx, node, key = \\"\\") {
const refs = this.__owl__.refs;
const ref1 = (el) => refs[\`myRef\`] = el;
const ref1 = singleRefSetter(refs, \`myRef\`);
const b2 = block2([ref1]);
const ctx1 = capture(ctx);
const b6 = comp1({prop: bind(this, ctx['method']),slots: markRaw({'default': {__render: slot1.bind(this), __ctx: ctx1}})}, key + \`__1\`, node, this, null);
Expand Down
Loading

0 comments on commit 0717fe2

Please sign in to comment.