From 09b7ef840f53b2918bb927791d3d920261dfe00b Mon Sep 17 00:00:00 2001 From: Richard Harris Date: Fri, 6 Sep 2019 09:21:31 -0400 Subject: [PATCH 1/3] fix code generation for if-else with static conditions - fixes #3505 --- .../compile/render_dom/wrappers/IfBlock.ts | 91 ++++++++++++------- .../if-block-static-with-else/_config.js | 3 + .../if-block-static-with-else/main.svelte | 5 + 3 files changed, 66 insertions(+), 33 deletions(-) create mode 100644 test/runtime/samples/if-block-static-with-else/_config.js create mode 100644 test/runtime/samples/if-block-static-with-else/main.svelte diff --git a/src/compiler/compile/render_dom/wrappers/IfBlock.ts b/src/compiler/compile/render_dom/wrappers/IfBlock.ts index 3c2899f355..38a223ead0 100644 --- a/src/compiler/compile/render_dom/wrappers/IfBlock.ts +++ b/src/compiler/compile/render_dom/wrappers/IfBlock.ts @@ -74,6 +74,7 @@ class IfBlockBranch extends Wrapper { export default class IfBlockWrapper extends Wrapper { node: IfBlock; branches: IfBlockBranch[]; + needs_update = false; var = 'if_block'; @@ -112,10 +113,16 @@ export default class IfBlockWrapper extends Wrapper { block.add_dependencies(node.expression.dependencies); if (branch.block.dependencies.size > 0) { + // the condition, or its contents, is dynamic is_dynamic = true; block.add_dependencies(branch.block.dependencies); } + if (branch.dependencies && branch.dependencies.length > 0) { + // the condition itself is dynamic + this.needs_update = true; + } + if (branch.block.has_intros) has_intros = true; if (branch.block.has_outros) has_outros = true; @@ -239,15 +246,29 @@ export default class IfBlockWrapper extends Wrapper { const current_block_type_and = has_else ? '' : `${current_block_type} && `; /* eslint-disable @typescript-eslint/indent,indent */ - block.builders.init.add_block(deindent` - function ${select_block_type}(changed, ctx) { - ${this.branches.map(({ dependencies, condition, snippet, block }) => condition - ? deindent` - ${snippet && `if ((${condition} == null) || ${dependencies.map(n => `changed.${n}`).join(' || ')}) ${condition} = !!(${snippet})`} - if (${condition}) return ${block.name};` - : `return ${block.name};`)} - } - `); + if (this.needs_update) { + block.builders.init.add_block(deindent` + function ${select_block_type}(changed, ctx) { + ${this.branches.map(({ dependencies, condition, snippet, block }) => condition + ? deindent` + ${snippet && ( + dependencies.length > 0 + ? `if ((${condition} == null) || ${dependencies.map(n => `changed.${n}`).join(' || ')}) ${condition} = !!(${snippet})` + : `if (${condition} == null) ${condition} = !!(${snippet})` + )} + if (${condition}) return ${block.name};` + : `return ${block.name};`)} + } + `); + } else { + block.builders.init.add_block(deindent` + function ${select_block_type}(changed, ctx) { + ${this.branches.map(({ condition, snippet, block }) => condition + ? `if (${snippet}) return ${block.name};` + : `return ${block.name};`)} + } + `); + } /* eslint-enable @typescript-eslint/indent,indent */ block.builders.init.add_block(deindent` @@ -261,32 +282,36 @@ export default class IfBlockWrapper extends Wrapper { `${if_name}${name}.m(${initial_mount_node}, ${anchor_node});` ); - const update_mount_node = this.get_update_mount_node(anchor); - - const change_block = deindent` - ${if_name}${name}.d(1); - ${name} = ${current_block_type_and}${current_block_type}(ctx); - if (${name}) { - ${name}.c(); - ${has_transitions && `@transition_in(${name}, 1);`} - ${name}.m(${update_mount_node}, ${anchor}); - } - `; + if (this.needs_update) { + const update_mount_node = this.get_update_mount_node(anchor); - if (dynamic) { - block.builders.update.add_block(deindent` - if (${current_block_type} === (${current_block_type} = ${select_block_type}(changed, ctx)) && ${name}) { - ${name}.p(changed, ctx); - } else { - ${change_block} - } - `); - } else { - block.builders.update.add_block(deindent` - if (${current_block_type} !== (${current_block_type} = ${select_block_type}(changed, ctx))) { - ${change_block} + const change_block = deindent` + ${if_name}${name}.d(1); + ${name} = ${current_block_type_and}${current_block_type}(ctx); + if (${name}) { + ${name}.c(); + ${has_transitions && `@transition_in(${name}, 1);`} + ${name}.m(${update_mount_node}, ${anchor}); } - `); + `; + + if (dynamic) { + block.builders.update.add_block(deindent` + if (${current_block_type} === (${current_block_type} = ${select_block_type}(changed, ctx)) && ${name}) { + ${name}.p(changed, ctx); + } else { + ${change_block} + } + `); + } else { + block.builders.update.add_block(deindent` + if (${current_block_type} !== (${current_block_type} = ${select_block_type}(changed, ctx))) { + ${change_block} + } + `); + } + } else if (dynamic) { + block.builders.update.add_line(`${name}.p(changed, ctx);`); } block.builders.destroy.add_line(`${if_name}${name}.d(${detaching});`); diff --git a/test/runtime/samples/if-block-static-with-else/_config.js b/test/runtime/samples/if-block-static-with-else/_config.js new file mode 100644 index 0000000000..8b2c6d2d66 --- /dev/null +++ b/test/runtime/samples/if-block-static-with-else/_config.js @@ -0,0 +1,3 @@ +export default { + html: 'eee' +}; diff --git a/test/runtime/samples/if-block-static-with-else/main.svelte b/test/runtime/samples/if-block-static-with-else/main.svelte new file mode 100644 index 0000000000..cd76a83510 --- /dev/null +++ b/test/runtime/samples/if-block-static-with-else/main.svelte @@ -0,0 +1,5 @@ +{#if "Eva".startsWith('E')} + eee +{:else} + rrr +{/if} \ No newline at end of file From a721d58ecefd7d17b6d87b55ff02026c097030ad Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Fri, 6 Sep 2019 17:58:11 -0400 Subject: [PATCH 2/3] extend fix to blocks with outros --- .../compile/render_dom/wrappers/IfBlock.ts | 153 +++++++++++------- .../EEE.svelte | 1 + .../RRR.svelte | 1 + .../_config.js | 3 + .../main.svelte | 10 ++ 5 files changed, 112 insertions(+), 56 deletions(-) create mode 100644 test/runtime/samples/if-block-static-with-else-and-outros/EEE.svelte create mode 100644 test/runtime/samples/if-block-static-with-else-and-outros/RRR.svelte create mode 100644 test/runtime/samples/if-block-static-with-else-and-outros/_config.js create mode 100644 test/runtime/samples/if-block-static-with-else-and-outros/main.svelte diff --git a/src/compiler/compile/render_dom/wrappers/IfBlock.ts b/src/compiler/compile/render_dom/wrappers/IfBlock.ts index 38a223ead0..d9bd38617b 100644 --- a/src/compiler/compile/render_dom/wrappers/IfBlock.ts +++ b/src/compiler/compile/render_dom/wrappers/IfBlock.ts @@ -340,6 +340,32 @@ export default class IfBlockWrapper extends Wrapper { block.add_variable(current_block_type_index); block.add_variable(name); + /* eslint-disable @typescript-eslint/indent,indent */ + if (this.needs_update) { + block.builders.init.add_block(deindent` + function ${select_block_type}(changed, ctx) { + ${this.branches.map(({ dependencies, condition, snippet, block }) => condition + ? deindent` + ${snippet && ( + dependencies.length > 0 + ? `if ((${condition} == null) || ${dependencies.map(n => `changed.${n}`).join(' || ')}) ${condition} = !!(${snippet})` + : `if (${condition} == null) ${condition} = !!(${snippet})` + )} + if (${condition}) return ${block.name};` + : `return ${block.name};`)} + } + `); + } else { + block.builders.init.add_block(deindent` + function ${select_block_type}(changed, ctx) { + ${this.branches.map(({ condition, snippet, block }) => condition + ? `if (${snippet}) return ${block.name};` + : `return ${block.name};`)} + } + `); + } + /* eslint-enable @typescript-eslint/indent,indent */ + /* eslint-disable @typescript-eslint/indent,indent */ block.builders.init.add_block(deindent` var ${if_block_creators} = [ @@ -348,14 +374,25 @@ export default class IfBlockWrapper extends Wrapper { var ${if_blocks} = []; - function ${select_block_type}(changed, ctx) { - ${this.branches.map(({ dependencies, condition, snippet }, i) => condition + ${this.needs_update ? deindent` - ${snippet && `if ((${condition} == null) || ${dependencies.map(n => `changed.${n}`).join(' || ')}) ${condition} = !!(${snippet})`} - if (${condition}) return ${String(i)};` - : `return ${i};`)} - ${!has_else && `return -1;`} - } + function ${select_block_type}(changed, ctx) { + ${this.branches.map(({ dependencies, condition, snippet }, i) => condition + ? deindent` + ${snippet && `if ((${condition} == null) || ${dependencies.map(n => `changed.${n}`).join(' || ')}) ${condition} = !!(${snippet})`} + if (${condition}) return ${String(i)};` + : `return ${i};`)} + ${!has_else && `return -1;`} + } + ` + : deindent` + function ${select_block_type}(changed, ctx) { + ${this.branches.map(({ condition, snippet }, i) => condition + ? `if (${snippet}) return ${String(i)};` + : `return ${i};`)} + ${!has_else && `return -1;`} + } + `} `); /* eslint-enable @typescript-eslint/indent,indent */ @@ -379,62 +416,66 @@ export default class IfBlockWrapper extends Wrapper { `${if_current_block_type_index}${if_blocks}[${current_block_type_index}].m(${initial_mount_node}, ${anchor_node});` ); - const update_mount_node = this.get_update_mount_node(anchor); - - const destroy_old_block = deindent` - @group_outros(); - @transition_out(${if_blocks}[${previous_block_index}], 1, 1, () => { - ${if_blocks}[${previous_block_index}] = null; - }); - @check_outros(); - `; + if (this.needs_update) { + const update_mount_node = this.get_update_mount_node(anchor); - const create_new_block = deindent` - ${name} = ${if_blocks}[${current_block_type_index}]; - if (!${name}) { - ${name} = ${if_blocks}[${current_block_type_index}] = ${if_block_creators}[${current_block_type_index}](ctx); - ${name}.c(); - } - ${has_transitions && `@transition_in(${name}, 1);`} - ${name}.m(${update_mount_node}, ${anchor}); - `; + const destroy_old_block = deindent` + @group_outros(); + @transition_out(${if_blocks}[${previous_block_index}], 1, 1, () => { + ${if_blocks}[${previous_block_index}] = null; + }); + @check_outros(); + `; - const change_block = has_else - ? deindent` - ${destroy_old_block} + const create_new_block = deindent` + ${name} = ${if_blocks}[${current_block_type_index}]; + if (!${name}) { + ${name} = ${if_blocks}[${current_block_type_index}] = ${if_block_creators}[${current_block_type_index}](ctx); + ${name}.c(); + } + ${has_transitions && `@transition_in(${name}, 1);`} + ${name}.m(${update_mount_node}, ${anchor}); + `; - ${create_new_block} - ` - : deindent` - if (${name}) { + const change_block = has_else + ? deindent` ${destroy_old_block} - } - if (~${current_block_type_index}) { ${create_new_block} - } else { - ${name} = null; - } - `; + ` + : deindent` + if (${name}) { + ${destroy_old_block} + } - if (dynamic) { - block.builders.update.add_block(deindent` - var ${previous_block_index} = ${current_block_type_index}; - ${current_block_type_index} = ${select_block_type}(changed, ctx); - if (${current_block_type_index} === ${previous_block_index}) { - ${if_current_block_type_index}${if_blocks}[${current_block_type_index}].p(changed, ctx); - } else { - ${change_block} - } - `); - } else { - block.builders.update.add_block(deindent` - var ${previous_block_index} = ${current_block_type_index}; - ${current_block_type_index} = ${select_block_type}(changed, ctx); - if (${current_block_type_index} !== ${previous_block_index}) { - ${change_block} - } - `); + if (~${current_block_type_index}) { + ${create_new_block} + } else { + ${name} = null; + } + `; + + if (dynamic) { + block.builders.update.add_block(deindent` + var ${previous_block_index} = ${current_block_type_index}; + ${current_block_type_index} = ${select_block_type}(changed, ctx); + if (${current_block_type_index} === ${previous_block_index}) { + ${if_current_block_type_index}${if_blocks}[${current_block_type_index}].p(changed, ctx); + } else { + ${change_block} + } + `); + } else { + block.builders.update.add_block(deindent` + var ${previous_block_index} = ${current_block_type_index}; + ${current_block_type_index} = ${select_block_type}(changed, ctx); + if (${current_block_type_index} !== ${previous_block_index}) { + ${change_block} + } + `); + } + } else if (dynamic) { + block.builders.update.add_line(`${name}.p(changed, ctx);`); } block.builders.destroy.add_line(deindent` diff --git a/test/runtime/samples/if-block-static-with-else-and-outros/EEE.svelte b/test/runtime/samples/if-block-static-with-else-and-outros/EEE.svelte new file mode 100644 index 0000000000..6132a64bc7 --- /dev/null +++ b/test/runtime/samples/if-block-static-with-else-and-outros/EEE.svelte @@ -0,0 +1 @@ +eee \ No newline at end of file diff --git a/test/runtime/samples/if-block-static-with-else-and-outros/RRR.svelte b/test/runtime/samples/if-block-static-with-else-and-outros/RRR.svelte new file mode 100644 index 0000000000..7242373249 --- /dev/null +++ b/test/runtime/samples/if-block-static-with-else-and-outros/RRR.svelte @@ -0,0 +1 @@ +rrr \ No newline at end of file diff --git a/test/runtime/samples/if-block-static-with-else-and-outros/_config.js b/test/runtime/samples/if-block-static-with-else-and-outros/_config.js new file mode 100644 index 0000000000..8b2c6d2d66 --- /dev/null +++ b/test/runtime/samples/if-block-static-with-else-and-outros/_config.js @@ -0,0 +1,3 @@ +export default { + html: 'eee' +}; diff --git a/test/runtime/samples/if-block-static-with-else-and-outros/main.svelte b/test/runtime/samples/if-block-static-with-else-and-outros/main.svelte new file mode 100644 index 0000000000..d23cb6bcf2 --- /dev/null +++ b/test/runtime/samples/if-block-static-with-else-and-outros/main.svelte @@ -0,0 +1,10 @@ + + +{#if "Eva".startsWith('E')} + +{:else} + +{/if} \ No newline at end of file From 397d6bffa362639f7a5a0fd4c8da3378412d6eb4 Mon Sep 17 00:00:00 2001 From: Richard Harris Date: Sat, 7 Sep 2019 09:12:19 -0400 Subject: [PATCH 3/3] fix --- .../compile/render_dom/wrappers/IfBlock.ts | 30 ++----------------- 1 file changed, 2 insertions(+), 28 deletions(-) diff --git a/src/compiler/compile/render_dom/wrappers/IfBlock.ts b/src/compiler/compile/render_dom/wrappers/IfBlock.ts index d9bd38617b..f56a0f0e50 100644 --- a/src/compiler/compile/render_dom/wrappers/IfBlock.ts +++ b/src/compiler/compile/render_dom/wrappers/IfBlock.ts @@ -264,7 +264,7 @@ export default class IfBlockWrapper extends Wrapper { block.builders.init.add_block(deindent` function ${select_block_type}(changed, ctx) { ${this.branches.map(({ condition, snippet, block }) => condition - ? `if (${snippet}) return ${block.name};` + ? `if (${snippet || condition}) return ${block.name};` : `return ${block.name};`)} } `); @@ -340,32 +340,6 @@ export default class IfBlockWrapper extends Wrapper { block.add_variable(current_block_type_index); block.add_variable(name); - /* eslint-disable @typescript-eslint/indent,indent */ - if (this.needs_update) { - block.builders.init.add_block(deindent` - function ${select_block_type}(changed, ctx) { - ${this.branches.map(({ dependencies, condition, snippet, block }) => condition - ? deindent` - ${snippet && ( - dependencies.length > 0 - ? `if ((${condition} == null) || ${dependencies.map(n => `changed.${n}`).join(' || ')}) ${condition} = !!(${snippet})` - : `if (${condition} == null) ${condition} = !!(${snippet})` - )} - if (${condition}) return ${block.name};` - : `return ${block.name};`)} - } - `); - } else { - block.builders.init.add_block(deindent` - function ${select_block_type}(changed, ctx) { - ${this.branches.map(({ condition, snippet, block }) => condition - ? `if (${snippet}) return ${block.name};` - : `return ${block.name};`)} - } - `); - } - /* eslint-enable @typescript-eslint/indent,indent */ - /* eslint-disable @typescript-eslint/indent,indent */ block.builders.init.add_block(deindent` var ${if_block_creators} = [ @@ -388,7 +362,7 @@ export default class IfBlockWrapper extends Wrapper { : deindent` function ${select_block_type}(changed, ctx) { ${this.branches.map(({ condition, snippet }, i) => condition - ? `if (${snippet}) return ${String(i)};` + ? `if (${snippet || condition}) return ${String(i)};` : `return ${i};`)} ${!has_else && `return -1;`} }