From 7fe3e4a85d73ac0cf5993e3ac02fade1a52304d0 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Sun, 29 Apr 2018 13:44:43 -0400 Subject: [PATCH 1/2] failing test for #1187 --- test/js/samples/deconflict-builtins/expected-bundle.js | 8 ++------ test/js/samples/deconflict-builtins/expected.js | 8 ++------ 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/test/js/samples/deconflict-builtins/expected-bundle.js b/test/js/samples/deconflict-builtins/expected-bundle.js index b677e75e292b..c92ae5aff03a 100644 --- a/test/js/samples/deconflict-builtins/expected-bundle.js +++ b/test/js/samples/deconflict-builtins/expected-bundle.js @@ -162,9 +162,7 @@ function create_main_fragment(component, ctx) { for (var i = 0; i < each_value.length; i += 1) { each_blocks[i] = create_each_block(component, assign(assign({}, ctx), { - each_value: each_value, - node: each_value[i], - node_index: i + node: each_value[i] })); } @@ -191,9 +189,7 @@ function create_main_fragment(component, ctx) { for (var i = 0; i < each_value.length; i += 1) { var each_context = assign(assign({}, ctx), { - each_value: each_value, - node: each_value[i], - node_index: i + node: each_value[i] }); if (each_blocks[i]) { diff --git a/test/js/samples/deconflict-builtins/expected.js b/test/js/samples/deconflict-builtins/expected.js index ce7e2fa52f8f..da3f6bb6b302 100644 --- a/test/js/samples/deconflict-builtins/expected.js +++ b/test/js/samples/deconflict-builtins/expected.js @@ -10,9 +10,7 @@ function create_main_fragment(component, ctx) { for (var i = 0; i < each_value.length; i += 1) { each_blocks[i] = create_each_block(component, assign(assign({}, ctx), { - each_value: each_value, - node: each_value[i], - node_index: i + node: each_value[i] })); } @@ -39,9 +37,7 @@ function create_main_fragment(component, ctx) { for (var i = 0; i < each_value.length; i += 1) { var each_context = assign(assign({}, ctx), { - each_value: each_value, - node: each_value[i], - node_index: i + node: each_value[i] }); if (each_blocks[i]) { From e7c62e9182aff84baa7c7bd0b951034e4344d6a4 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Sun, 28 Oct 2018 11:11:06 -0400 Subject: [PATCH 2/2] only add list/index to each block context if necessary --- src/compile/render-dom/Block.ts | 4 ++++ src/compile/render-dom/Renderer.ts | 1 + src/compile/render-dom/wrappers/EachBlock.ts | 21 +++++++++++-------- .../render-dom/wrappers/Element/Binding.ts | 9 ++++++++ .../wrappers/InlineComponent/index.ts | 9 ++++++++ .../debug-foo-bar-baz-things/expected.js | 2 -- test/js/samples/debug-foo/expected.js | 2 -- .../each-block-changed-check/expected.js | 1 - .../each-block-keyed-animated/expected.js | 2 -- test/js/samples/each-block-keyed/expected.js | 2 -- 10 files changed, 35 insertions(+), 18 deletions(-) diff --git a/src/compile/render-dom/Block.ts b/src/compile/render-dom/Block.ts index a95dce784354..c7ca63605cf6 100644 --- a/src/compile/render-dom/Block.ts +++ b/src/compile/render-dom/Block.ts @@ -3,6 +3,7 @@ import deindent from '../../utils/deindent'; import { escape } from '../../utils/stringify'; import Renderer from './Renderer'; import Wrapper from './wrappers/shared/Wrapper'; +import EachBlockWrapper from './wrappers/EachBlock'; export interface BlockOptions { parent?: Block; @@ -11,6 +12,7 @@ export interface BlockOptions { comment?: string; key?: string; bindings?: Map string>; + contextOwners?: Map; dependencies?: Set; } @@ -28,6 +30,7 @@ export default class Block { dependencies: Set; bindings: Map string>; + contextOwners: Map; builders: { init: CodeBuilder; @@ -74,6 +77,7 @@ export default class Block { this.dependencies = new Set(); this.bindings = options.bindings; + this.contextOwners = options.contextOwners; this.builders = { init: new CodeBuilder(), diff --git a/src/compile/render-dom/Renderer.ts b/src/compile/render-dom/Renderer.ts index 9ed621045735..7e8632f34100 100644 --- a/src/compile/render-dom/Renderer.ts +++ b/src/compile/render-dom/Renderer.ts @@ -47,6 +47,7 @@ export default class Renderer { key: null, bindings: new Map(), + contextOwners: new Map(), dependencies: new Set(), }); diff --git a/src/compile/render-dom/wrappers/EachBlock.ts b/src/compile/render-dom/wrappers/EachBlock.ts index 22515206c15d..b9e33bf52dab 100644 --- a/src/compile/render-dom/wrappers/EachBlock.ts +++ b/src/compile/render-dom/wrappers/EachBlock.ts @@ -53,7 +53,6 @@ export default class EachBlockWrapper extends Wrapper { node: EachBlock; fragment: FragmentWrapper; else?: ElseBlockWrapper; - var: string; vars: { anchor: string; create_each_block: string; @@ -64,6 +63,12 @@ export default class EachBlockWrapper extends Wrapper { mountOrIntro: string; } + contextProps: string[]; + indexName: string; + + var = 'each'; + hasBinding = false; + constructor( renderer: Renderer, block: Block, @@ -75,8 +80,6 @@ export default class EachBlockWrapper extends Wrapper { super(renderer, block, parent, node); this.cannotUseInnerHTML(); - this.var = 'each'; - const { dependencies } = node.expression; block.addDependencies(dependencies); @@ -85,7 +88,8 @@ export default class EachBlockWrapper extends Wrapper { name: renderer.component.getUniqueName('create_each_block'), key: node.key, // TODO... - bindings: new Map(block.bindings) + bindings: new Map(block.bindings), + contextOwners: new Map(block.contextOwners) }); // TODO this seems messy @@ -94,6 +98,8 @@ export default class EachBlockWrapper extends Wrapper { this.indexName = this.node.index || renderer.component.getUniqueName(`${this.node.context}_index`); node.contexts.forEach(prop => { + this.block.contextOwners.set(prop.key.name, this); + // TODO this doesn't feel great this.block.bindings.set(prop.key.name, () => `ctx.${this.vars.each_block_value}[ctx.${this.indexName}]${prop.tail}`); }); @@ -164,11 +170,8 @@ export default class EachBlockWrapper extends Wrapper { this.contextProps = this.node.contexts.map(prop => `child_ctx.${prop.key.name} = list[i]${prop.tail};`); - // TODO only add these if necessary - this.contextProps.push( - `child_ctx.${this.vars.each_block_value} = list;`, - `child_ctx.${this.indexName} = i;` - ); + if (this.hasBinding) this.contextProps.push(`child_ctx.${this.vars.each_block_value} = list;`); + if (this.hasBinding || this.node.index) this.contextProps.push(`child_ctx.${this.indexName} = i;`); const { snippet } = this.node.expression; diff --git a/src/compile/render-dom/wrappers/Element/Binding.ts b/src/compile/render-dom/wrappers/Element/Binding.ts index f54814d17049..959d6171113d 100644 --- a/src/compile/render-dom/wrappers/Element/Binding.ts +++ b/src/compile/render-dom/wrappers/Element/Binding.ts @@ -42,6 +42,15 @@ export default class BindingWrapper { parent.renderer.component.indirectDependencies.set(prop, new Set()); }); } + + if (node.isContextual) { + // we need to ensure that the each block creates a context including + // the list and the index, if they're not otherwise referenced + const { name } = getObject(this.node.value.node); + const eachBlock = block.contextOwners.get(name); + + eachBlock.hasBinding = true; + } } isReadOnlyMediaAttribute() { diff --git a/src/compile/render-dom/wrappers/InlineComponent/index.ts b/src/compile/render-dom/wrappers/InlineComponent/index.ts index 01ac827e2267..fc0a422241cf 100644 --- a/src/compile/render-dom/wrappers/InlineComponent/index.ts +++ b/src/compile/render-dom/wrappers/InlineComponent/index.ts @@ -41,6 +41,15 @@ export default class InlineComponentWrapper extends Wrapper { }); this.node.bindings.forEach(binding => { + if (binding.isContextual) { + // we need to ensure that the each block creates a context including + // the list and the index, if they're not otherwise referenced + const { name } = getObject(binding.value.node); + const eachBlock = block.contextOwners.get(name); + + eachBlock.hasBinding = true; + } + block.addDependencies(binding.value.dependencies); }); diff --git a/test/js/samples/debug-foo-bar-baz-things/expected.js b/test/js/samples/debug-foo-bar-baz-things/expected.js index cc00d13d3fbf..0ecaab9c54f1 100644 --- a/test/js/samples/debug-foo-bar-baz-things/expected.js +++ b/test/js/samples/debug-foo-bar-baz-things/expected.js @@ -6,8 +6,6 @@ const file = undefined; function get_each_context(ctx, list, i) { const child_ctx = Object.create(ctx); child_ctx.thing = list[i]; - child_ctx.each_value = list; - child_ctx.thing_index = i; return child_ctx; } diff --git a/test/js/samples/debug-foo/expected.js b/test/js/samples/debug-foo/expected.js index 091838cde808..564a18df89e4 100644 --- a/test/js/samples/debug-foo/expected.js +++ b/test/js/samples/debug-foo/expected.js @@ -6,8 +6,6 @@ const file = undefined; function get_each_context(ctx, list, i) { const child_ctx = Object.create(ctx); child_ctx.thing = list[i]; - child_ctx.each_value = list; - child_ctx.thing_index = i; return child_ctx; } diff --git a/test/js/samples/each-block-changed-check/expected.js b/test/js/samples/each-block-changed-check/expected.js index e71b90106e5a..ea2e52770618 100644 --- a/test/js/samples/each-block-changed-check/expected.js +++ b/test/js/samples/each-block-changed-check/expected.js @@ -4,7 +4,6 @@ import { append, assign, createElement, createText, destroyEach, detachAfter, de function get_each_context(ctx, list, i) { const child_ctx = Object.create(ctx); child_ctx.comment = list[i]; - child_ctx.each_value = list; child_ctx.i = i; return child_ctx; } diff --git a/test/js/samples/each-block-keyed-animated/expected.js b/test/js/samples/each-block-keyed-animated/expected.js index 5e522bd90809..d469562fadcf 100644 --- a/test/js/samples/each-block-keyed-animated/expected.js +++ b/test/js/samples/each-block-keyed-animated/expected.js @@ -18,8 +18,6 @@ function foo(node, animation, params) { function get_each_context(ctx, list, i) { const child_ctx = Object.create(ctx); child_ctx.thing = list[i]; - child_ctx.each_value = list; - child_ctx.thing_index = i; return child_ctx; } diff --git a/test/js/samples/each-block-keyed/expected.js b/test/js/samples/each-block-keyed/expected.js index 353dde2568d1..94fce558010a 100644 --- a/test/js/samples/each-block-keyed/expected.js +++ b/test/js/samples/each-block-keyed/expected.js @@ -4,8 +4,6 @@ import { append, assign, blankObject, createComment, createElement, createText, function get_each_context(ctx, list, i) { const child_ctx = Object.create(ctx); child_ctx.thing = list[i]; - child_ctx.each_value = list; - child_ctx.thing_index = i; return child_ctx; }