Merge pull request #1383 from sveltejs/gh-1187-b

only add list/index to each block context if necessary

Author: Rich-Harris

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, () => string>;
+	contextOwners?: Map<string, EachBlockWrapper>;
 	dependencies?: Set<string>;
 }
 
@@ -28,6 +30,7 @@ export default class Block {
 	dependencies: Set<string>;
 
 	bindings: Map<string, () => string>;
+	contextOwners: Map<string, EachBlockWrapper>;
 
 	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(),

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(),
 		});

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: <string>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;
 

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() {

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);
 		});
 

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;
 }
 

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;
 }
 

test/js/samples/deconflict-builtins/expected.js

@@ -4,8 +4,6 @@ import { append, assign, createComment, createElement, createText, destroyEach,
 function get_each_context(ctx, list, i) {
 	const child_ctx = Object.create(ctx);
 	child_ctx.node = list[i];
-	child_ctx.each_value = list;
-	child_ctx.node_index = i;
 	return child_ctx;
 }
 

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;
 }

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;
 }
 

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;
 }