From d7de25e867e4ceae033f754d2ccb2c440bb7ad0e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Wed, 15 Dec 2021 14:34:58 +0100 Subject: [PATCH] [FIX] compiler: properly handle tags in some cases The problem was that the compiler is based on the assumption that the multi block received by the parser only occurs in some cases where the structure of the template require a multi block, and it does not work when we have random multiblock elsewhere. We could fix the issue by modifying the code generator code to support these usecases, or by simply removing these cases in the parser. Since this seems more efficient, this is the approach taken by this commit. Note that it was a good opportunity to simplify the parser. --- src/compiler/parser.ts | 63 +++++--------- tests/blockdom/multi.test.ts | 7 ++ .../simple_templates.test.ts.snap | 33 +++++++ tests/compiler/parser.test.ts | 87 +++++++++++++++++++ tests/compiler/simple_templates.test.ts | 17 ++++ 5 files changed, 166 insertions(+), 41 deletions(-) diff --git a/src/compiler/parser.ts b/src/compiler/parser.ts index ebe5f0e4..72068c59 100644 --- a/src/compiler/parser.ts +++ b/src/compiler/parser.ts @@ -217,24 +217,7 @@ function parseTNode(node: Element, ctx: ParsingContext): AST | null { if (node.tagName !== "t") { return null; } - const children: AST[] = []; - for (let child of node.childNodes) { - const ast = parseNode(child, ctx); - if (ast) { - children.push(ast); - } - } - switch (children.length) { - case 0: - return null; - case 1: - return children[0]; - default: - return { - type: ASTType.Multi, - content: children, - }; - } + return parseChildNodes(node, ctx); } // ----------------------------------------------------------------------------- @@ -299,7 +282,6 @@ function parseDOMNode(node: Element, ctx: ParsingContext): AST | null { return null; } ctx = Object.assign({}, ctx); - const children: AST[] = []; if (tagName === "pre") { ctx.inPreTag = true; } @@ -309,12 +291,7 @@ function parseDOMNode(node: Element, ctx: ParsingContext): AST | null { const ref = node.getAttribute("t-ref"); node.removeAttribute("t-ref"); - for (let child of node.childNodes) { - const ast = parseNode(child, ctx); - if (ast) { - children.push(ast); - } - } + const children = parseChildren(node, ctx); const nodeAttrsNames = node.getAttributeNames(); const attrs: ASTDomNode["attrs"] = {}; @@ -600,13 +577,7 @@ function parseTCall(node: Element, ctx: ParsingContext): AST | null { }; } } - const body: AST[] = []; - for (let child of node.childNodes) { - const ast = parseNode(child, ctx); - if (ast) { - body.push(ast); - } - } + const body = parseChildren(node, ctx); return { type: ASTType.TCall, @@ -687,13 +658,7 @@ function parseTSetNode(node: Element, ctx: ParsingContext): AST | null { const defaultValue = node.innerHTML === node.textContent ? node.textContent || null : null; let body: AST[] | null = null; if (node.textContent !== node.innerHTML) { - body = []; - for (let child of node.childNodes) { - let childAst = parseNode(child, ctx); - if (childAst) { - body.push(childAst); - } - } + body = parseChildren(node, ctx); } return { type: ASTType.TSet, name, value, defaultValue, body }; } @@ -844,14 +809,30 @@ function parseTTranslation(node: Element, ctx: ParsingContext): AST | null { // helpers // ----------------------------------------------------------------------------- -function parseChildNodes(node: Element, ctx: ParsingContext): AST | null { +/** + * Parse all the child nodes of a given node and return a list of ast elements + */ +function parseChildren(node: Node, ctx: ParsingContext): AST[] { const children: AST[] = []; for (let child of node.childNodes) { const childAst = parseNode(child, ctx); if (childAst) { - children.push(childAst); + if (childAst.type === ASTType.Multi) { + children.push(...childAst.content); + } else { + children.push(childAst); + } } } + return children; +} + +/** + * Parse all the child nodes of a given node and return an ast if possible. + * In the case there are multiple children, they are wrapped in a astmulti. + */ +function parseChildNodes(node: Node, ctx: ParsingContext): AST | null { + const children = parseChildren(node, ctx); switch (children.length) { case 0: return null; diff --git a/tests/blockdom/multi.test.ts b/tests/blockdom/multi.test.ts index efd2c253..2ec3a694 100644 --- a/tests/blockdom/multi.test.ts +++ b/tests/blockdom/multi.test.ts @@ -76,4 +76,11 @@ describe("multi blocks", () => { mount(text(multi([text("a"), text("b")]) as any), fixture); expect(fixture.innerHTML).toBe("ab"); }); + + test("multi inside a block", async () => { + const block = createBlock("
"); + const tree = block([], [multi([text("foo"), text("bar")])]); + mount(tree, fixture); + expect(fixture.innerHTML).toBe("
foobar
"); + }); }); diff --git a/tests/compiler/__snapshots__/simple_templates.test.ts.snap b/tests/compiler/__snapshots__/simple_templates.test.ts.snap index e9115904..ef9ad4e2 100644 --- a/tests/compiler/__snapshots__/simple_templates.test.ts.snap +++ b/tests/compiler/__snapshots__/simple_templates.test.ts.snap @@ -308,6 +308,39 @@ exports[`simple templates, mostly static t-esc in dom node, variations 2`] = ` }" `; +exports[`simple templates, mostly static template with multiple t tag with multiple content 1`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + + let block1 = createBlock(\`
Loading
\`); + + return function template(ctx, node, key = \\"\\") { + let d1 = ctx['a']; + let d2 = ctx['b']; + let d3 = ctx['c']; + return block1([d1, d2, d3]); + } +}" +`; + +exports[`simple templates, mostly static template with t tag with multiple content 1`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + + let block1 = createBlock(\`
Loading
\`); + + return function template(ctx, node, key = \\"\\") { + let b2; + if (false) { + b2 = text(\`\`); + } + return block1([], [b2]); + } +}" +`; + exports[`simple templates, mostly static two t-escs next to each other 1`] = ` "function anonymous(bdom, helpers ) { diff --git a/tests/compiler/parser.test.ts b/tests/compiler/parser.test.ts index 366d044b..3eb9437e 100644 --- a/tests/compiler/parser.test.ts +++ b/tests/compiler/parser.test.ts @@ -149,6 +149,93 @@ describe("qweb parser", () => { }); }); + test("dom node with t multi inside", async () => { + const template = `
Loading
`; + expect(parse(template)).toEqual({ + type: ASTType.DomNode, + tag: "div", + dynamicTag: null, + attrs: {}, + on: {}, + ref: null, + model: null, + ns: null, + content: [ + { type: ASTType.Text, value: "Loading" }, + { type: ASTType.TEsc, expr: "abc", defaultValue: "" }, + ], + }); + }); + + test("dom node with multiple t multi inside", async () => { + const template = ` +
+ + + + Loading + +
`; + expect(parse(template)).toEqual({ + type: ASTType.DomNode, + tag: "div", + dynamicTag: null, + attrs: {}, + on: {}, + ref: null, + model: null, + ns: null, + content: [ + { type: ASTType.TEsc, expr: "a", defaultValue: "" }, + { type: ASTType.TEsc, expr: "b", defaultValue: "" }, + { type: ASTType.Text, value: "Loading" }, + { type: ASTType.TEsc, expr: "c", defaultValue: "" }, + ], + }); + }); + + test("dom node with t multi inside", async () => { + const template = `
Loading
`; + expect(parse(template)).toEqual({ + type: ASTType.DomNode, + tag: "div", + dynamicTag: null, + attrs: {}, + on: {}, + ref: null, + model: null, + ns: null, + content: [ + { type: ASTType.Text, value: "Loading" }, + { type: ASTType.TEsc, expr: "abc", defaultValue: "" }, + ], + }); + }); + + test("dom node with two t multi inside", async () => { + const template = ` +
+ + +
`; + expect(parse(template)).toEqual({ + type: ASTType.DomNode, + tag: "div", + dynamicTag: null, + attrs: {}, + on: {}, + ref: null, + model: null, + ns: null, + content: [ + { type: ASTType.TEsc, expr: "a", defaultValue: "" }, + { type: ASTType.TEsc, expr: "b", defaultValue: "" }, + { type: ASTType.TEsc, expr: "c", defaultValue: "" }, + { type: ASTType.TEsc, expr: "d", defaultValue: "" }, + ], + }); + }); + test("dom node next to text node", async () => { expect(parse("some text")).toEqual({ type: ASTType.Multi, diff --git a/tests/compiler/simple_templates.test.ts b/tests/compiler/simple_templates.test.ts index cb6cfd4e..3e62da8e 100644 --- a/tests/compiler/simple_templates.test.ts +++ b/tests/compiler/simple_templates.test.ts @@ -137,4 +137,21 @@ describe("simple templates, mostly static", () => { const template = ''; expect(renderToString(template, { v: "from context" })).toBe("text from context"); }); + + test("template with t tag with multiple content", () => { + const template = `
Loading
`; + expect(renderToString(template)).toBe("
Loading
"); + }); + + test("template with multiple t tag with multiple content", () => { + const template = ` +
+ + + + Loading + +
`; + expect(renderToString(template, { a: "a", b: "b", c: "c" })).toBe("
abLoadingc
"); + }); });