From 6af23ba88c0a0e8bc83bd4393a2f96b705667533 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 27 Jun 2019 19:59:11 -0400 Subject: [PATCH] Fix contextual bind:this (#2806) --- .../render_dom/wrappers/Element/index.ts | 34 +------- .../wrappers/InlineComponent/index.ts | 38 +-------- .../render_dom/wrappers/shared/bind_this.ts | 80 +++++++++++++++++++ src/runtime/internal/scheduler.ts | 2 +- .../Foo.svelte | 7 ++ .../_config.js | 12 +++ .../main.svelte | 13 +++ .../_config.js | 12 +++ .../main.svelte | 11 +++ 9 files changed, 141 insertions(+), 68 deletions(-) create mode 100644 src/compiler/compile/render_dom/wrappers/shared/bind_this.ts create mode 100644 test/runtime/samples/binding-this-each-block-property-component/Foo.svelte create mode 100644 test/runtime/samples/binding-this-each-block-property-component/_config.js create mode 100644 test/runtime/samples/binding-this-each-block-property-component/main.svelte create mode 100644 test/runtime/samples/binding-this-each-block-property/_config.js create mode 100644 test/runtime/samples/binding-this-each-block-property/main.svelte diff --git a/src/compiler/compile/render_dom/wrappers/Element/index.ts b/src/compiler/compile/render_dom/wrappers/Element/index.ts index 9179225e29..9b14d1cd6c 100644 --- a/src/compiler/compile/render_dom/wrappers/Element/index.ts +++ b/src/compiler/compile/render_dom/wrappers/Element/index.ts @@ -19,6 +19,7 @@ import add_event_handlers from '../shared/add_event_handlers'; import add_actions from '../shared/add_actions'; import create_debugging_comment from '../shared/create_debugging_comment'; import { get_context_merger } from '../shared/get_context_merger'; +import bind_this from '../shared/bind_this'; const events = [ { @@ -540,38 +541,9 @@ export default class ElementWrapper extends Wrapper { const this_binding = this.bindings.find(b => b.node.name === 'this'); if (this_binding) { - const name = renderer.component.get_unique_name(`${this.var}_binding`); + const binding_callback = bind_this(renderer.component, block, this_binding.node, this.var); - renderer.component.add_var({ - name, - internal: true, - referenced: true - }); - - const { handler, object } = this_binding; - - const args = []; - for (const arg of handler.contextual_dependencies) { - args.push(arg); - block.add_variable(arg, `ctx.${arg}`); - } - - renderer.component.partly_hoisted.push(deindent` - function ${name}(${['$$node', 'check'].concat(args).join(', ')}) { - ${handler.snippet ? `if ($$node || (!$$node && ${handler.snippet} === check)) ` : ''}${handler.mutation} - ${renderer.component.invalidate(object)}; - } - `); - - block.builders.mount.add_line(`@add_binding_callback(() => ctx.${name}(${[this.var, 'null'].concat(args).join(', ')}));`); - block.builders.destroy.add_line(`ctx.${name}(${['null', this.var].concat(args).join(', ')});`); - block.builders.update.add_line(deindent` - if (changed.items) { - ctx.${name}(${['null', this.var].concat(args).join(', ')}); - ${args.map(a => `${a} = ctx.${a}`).join(', ')}; - ctx.${name}(${[this.var, 'null'].concat(args).join(', ')}); - }` - ); + block.builders.mount.add_line(binding_callback); } } diff --git a/src/compiler/compile/render_dom/wrappers/InlineComponent/index.ts b/src/compiler/compile/render_dom/wrappers/InlineComponent/index.ts index 6045daaf36..ece8abf3b0 100644 --- a/src/compiler/compile/render_dom/wrappers/InlineComponent/index.ts +++ b/src/compiler/compile/render_dom/wrappers/InlineComponent/index.ts @@ -9,11 +9,11 @@ import add_to_set from '../../../utils/add_to_set'; import deindent from '../../../utils/deindent'; import Attribute from '../../../nodes/Attribute'; import get_object from '../../../utils/get_object'; -import flatten_reference from '../../../utils/flatten_reference'; import create_debugging_comment from '../shared/create_debugging_comment'; import { get_context_merger } from '../shared/get_context_merger'; import EachBlock from '../../../nodes/EachBlock'; import TemplateScope from '../../../nodes/shared/TemplateScope'; +import bind_this from '../shared/bind_this'; export default class InlineComponentWrapper extends Wrapper { var: string; @@ -252,41 +252,7 @@ export default class InlineComponentWrapper extends Wrapper { component.has_reactive_assignments = true; if (binding.name === 'this') { - const fn = component.get_unique_name(`${this.var}_binding`); - - component.add_var({ - name: fn, - internal: true, - referenced: true - }); - - let lhs; - let object; - - if (binding.is_contextual && binding.expression.node.type === 'Identifier') { - // bind:x={y} — we can't just do `y = x`, we need to - // to `array[index] = x; - const { name } = binding.expression.node; - const { snippet } = block.bindings.get(name); - lhs = snippet; - - // TODO we need to invalidate... something - } else { - object = flatten_reference(binding.expression.node).name; - lhs = component.source.slice(binding.expression.node.start, binding.expression.node.end).trim(); - } - - const contextual_dependencies = [...binding.expression.contextual_dependencies]; - - component.partly_hoisted.push(deindent` - function ${fn}(${['$$component', ...contextual_dependencies].join(', ')}) { - ${lhs} = $$component; - ${object && component.invalidate(object)} - } - `); - - block.builders.destroy.add_line(`ctx.${fn}(null);`); - return `@add_binding_callback(() => ctx.${fn}(${[this.var, ...contextual_dependencies.map(name => `ctx.${name}`)].join(', ')}));`; + return bind_this(component, block, binding, this.var); } const name = component.get_unique_name(`${this.var}_${binding.name}_binding`); diff --git a/src/compiler/compile/render_dom/wrappers/shared/bind_this.ts b/src/compiler/compile/render_dom/wrappers/shared/bind_this.ts new file mode 100644 index 0000000000..3d08cfaff5 --- /dev/null +++ b/src/compiler/compile/render_dom/wrappers/shared/bind_this.ts @@ -0,0 +1,80 @@ +import flatten_reference from '../../../utils/flatten_reference'; +import deindent from '../../../utils/deindent'; +import Component from '../../../Component'; +import Block from '../../Block'; +import Binding from '../../../nodes/Binding'; + +export default function bind_this(component: Component, block: Block, binding: Binding, variable: string) { + const fn = component.get_unique_name(`${variable}_binding`); + + component.add_var({ + name: fn, + internal: true, + referenced: true + }); + + let lhs; + let object; + + if (binding.is_contextual && binding.expression.node.type === 'Identifier') { + // bind:x={y} — we can't just do `y = x`, we need to + // to `array[index] = x; + const { name } = binding.expression.node; + const { snippet } = block.bindings.get(name); + lhs = snippet; + + // TODO we need to invalidate... something + } else { + object = flatten_reference(binding.expression.node).name; + lhs = component.source.slice(binding.expression.node.start, binding.expression.node.end).trim(); + } + + const contextual_dependencies = Array.from(binding.expression.contextual_dependencies); + + if (contextual_dependencies.length) { + component.partly_hoisted.push(deindent` + function ${fn}(${['$$value', ...contextual_dependencies].join(', ')}) { + if (${lhs} === $$value) return; + ${lhs} = $$value; + ${object && component.invalidate(object)} + } + `); + + const args = []; + for (const arg of contextual_dependencies) { + args.push(arg); + block.add_variable(arg, `ctx.${arg}`); + } + + const assign = block.get_unique_name(`assign_${variable}`); + const unassign = block.get_unique_name(`unassign_${variable}`); + + block.builders.init.add_block(deindent` + const ${assign} = () => ctx.${fn}(${[variable].concat(args).join(', ')}); + const ${unassign} = () => ctx.${fn}(${['null'].concat(args).join(', ')}); + `); + + const condition = Array.from(contextual_dependencies).map(name => `${name} !== ctx.${name}`).join(' || '); + + block.builders.update.add_line(deindent` + if (${condition}) { + ${unassign}(); + ${args.map(a => `${a} = ctx.${a}`).join(', ')}; + @add_binding_callback(${assign}); + }` + ); + + block.builders.destroy.add_line(`${unassign}();`); + return `@add_binding_callback(${assign});`; + } + + component.partly_hoisted.push(deindent` + function ${fn}($$value) { + ${lhs} = $$value; + ${object && component.invalidate(object)} + } + `); + + block.builders.destroy.add_line(`ctx.${fn}(null);`); + return `@add_binding_callback(() => ctx.${fn}(${variable}));`; +} \ No newline at end of file diff --git a/src/runtime/internal/scheduler.ts b/src/runtime/internal/scheduler.ts index a26a4f8c33..9e1b4280bf 100644 --- a/src/runtime/internal/scheduler.ts +++ b/src/runtime/internal/scheduler.ts @@ -46,7 +46,7 @@ export function flush() { update(component.$$); } - while (binding_callbacks.length) binding_callbacks.shift()(); + while (binding_callbacks.length) binding_callbacks.pop()(); // then, once components are updated, call // afterUpdate functions. This may cause diff --git a/test/runtime/samples/binding-this-each-block-property-component/Foo.svelte b/test/runtime/samples/binding-this-each-block-property-component/Foo.svelte new file mode 100644 index 0000000000..dc8b5f206c --- /dev/null +++ b/test/runtime/samples/binding-this-each-block-property-component/Foo.svelte @@ -0,0 +1,7 @@ + + +

\ No newline at end of file diff --git a/test/runtime/samples/binding-this-each-block-property-component/_config.js b/test/runtime/samples/binding-this-each-block-property-component/_config.js new file mode 100644 index 0000000000..947ec17929 --- /dev/null +++ b/test/runtime/samples/binding-this-each-block-property-component/_config.js @@ -0,0 +1,12 @@ +export default { + html: ``, + + async test({ assert, component, target }) { + component.visible = true; + assert.htmlEqual(target.innerHTML, ` +

a

+ `); + + assert.ok(component.items[0].ref.isFoo()); + } +}; diff --git a/test/runtime/samples/binding-this-each-block-property-component/main.svelte b/test/runtime/samples/binding-this-each-block-property-component/main.svelte new file mode 100644 index 0000000000..257aa18a91 --- /dev/null +++ b/test/runtime/samples/binding-this-each-block-property-component/main.svelte @@ -0,0 +1,13 @@ + + +{#if visible} + {#each items as item} + {item.value} + {/each} +{/if} diff --git a/test/runtime/samples/binding-this-each-block-property/_config.js b/test/runtime/samples/binding-this-each-block-property/_config.js new file mode 100644 index 0000000000..15ad4be5db --- /dev/null +++ b/test/runtime/samples/binding-this-each-block-property/_config.js @@ -0,0 +1,12 @@ +export default { + html: ``, + + async test({ assert, component, target }) { + component.visible = true; + assert.htmlEqual(target.innerHTML, ` +
a
+ `); + + assert.equal(component.items[0].ref, target.querySelector('div')); + } +}; diff --git a/test/runtime/samples/binding-this-each-block-property/main.svelte b/test/runtime/samples/binding-this-each-block-property/main.svelte new file mode 100644 index 0000000000..73ede7ab75 --- /dev/null +++ b/test/runtime/samples/binding-this-each-block-property/main.svelte @@ -0,0 +1,11 @@ + + +{#if visible} + {#each items as item} +
{item.value}
+ {/each} +{/if}