From e22457f6137ef6e9d259d07cc254d3b651a29365 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Mon, 31 Dec 2018 10:48:06 -0500 Subject: [PATCH] fix hoisting of functions that reference imported values - fixes #1933 --- src/compile/Component.ts | 3 +- src/compile/nodes/Animation.ts | 3 +- src/compile/nodes/shared/Expression.ts | 14 +++-- src/compile/render-dom/index.ts | 18 +++--- .../render-dom/wrappers/Element/index.ts | 5 +- .../samples/animation-js-easing/_config.js | 56 +++++++++++++++++++ .../samples/animation-js-easing/easing.js | 3 + .../samples/animation-js-easing/main.html | 22 ++++++++ 8 files changed, 104 insertions(+), 20 deletions(-) create mode 100644 test/runtime/samples/animation-js-easing/_config.js create mode 100644 test/runtime/samples/animation-js-easing/easing.js create mode 100644 test/runtime/samples/animation-js-easing/main.html diff --git a/src/compile/Component.ts b/src/compile/Component.ts index a36a8e3e96..3118d8fa08 100644 --- a/src/compile/Component.ts +++ b/src/compile/Component.ts @@ -683,7 +683,7 @@ export default class Component { // reference instance variables other than other // hoistable functions. TODO others? - const { hoistable_names, hoistable_nodes } = this; + const { hoistable_names, hoistable_nodes, imported_declarations } = this; const top_level_function_declarations = new Map(); @@ -738,6 +738,7 @@ export default class Component { if (owner === instance_scope) { if (name === fn_declaration.id.name) return; if (hoistable_names.has(name)) return; + if (imported_declarations.has(name)) return; if (top_level_function_declarations.has(name)) { const other_declaration = top_level_function_declarations.get(name); diff --git a/src/compile/nodes/Animation.ts b/src/compile/nodes/Animation.ts index 5470b34fe7..faf964b06c 100644 --- a/src/compile/nodes/Animation.ts +++ b/src/compile/nodes/Animation.ts @@ -1,12 +1,13 @@ import Node from './shared/Node'; import Expression from './shared/Expression'; +import Component from '../Component'; export default class Animation extends Node { type: 'Animation'; name: string; expression: Expression; - constructor(component, parent, scope, info) { + constructor(component: Component, parent, scope, info) { super(component, parent, scope, info); component.warn_if_undefined(info, scope); diff --git a/src/compile/nodes/shared/Expression.ts b/src/compile/nodes/shared/Expression.ts index be300317e3..44e3ab7168 100644 --- a/src/compile/nodes/shared/Expression.ts +++ b/src/compile/nodes/shared/Expression.ts @@ -264,17 +264,19 @@ export default class Expression { code.overwrite(node.start, node.end, dirty.map(n => `$$invalidate('${n}', ${n})`).join('; ')); } else { names.forEach(name => { - if (!scope.declarations.has(name)) { - pending_assignments.add(name); - } + if (scope.declarations.has(name)) return; + if (component.imported_declarations.has(name)) return; + + pending_assignments.add(name); }); } } else if (node.type === 'UpdateExpression') { const { name } = getObject(node.argument); - if (!scope.declarations.has(name)) { - pending_assignments.add(name); - } + if (scope.declarations.has(name)) return; + if (component.imported_declarations.has(name)) return; + + pending_assignments.add(name); } } else { if (node.type === 'AssignmentExpression') { diff --git a/src/compile/render-dom/index.ts b/src/compile/render-dom/index.ts index 28b7521f92..53750232b0 100644 --- a/src/compile/render-dom/index.ts +++ b/src/compile/render-dom/index.ts @@ -174,10 +174,11 @@ export default function dom( code.overwrite(node.start, node.end, dirty.map(n => `$$invalidate('${n}', ${n})`).join('; ')); } else { names.forEach(name => { - if (scope.findOwner(name) === component.instance_scope) { - pending_assignments.add(name); - component.has_reactive_assignments = true; - } + if (component.imported_declarations.has(name)) return; + if (scope.findOwner(name) !== component.instance_scope) return; + + pending_assignments.add(name); + component.has_reactive_assignments = true; }); } } @@ -185,10 +186,11 @@ export default function dom( else if (node.type === 'UpdateExpression') { const { name } = getObject(node.argument); - if (scope.findOwner(name) === component.instance_scope) { - pending_assignments.add(name); - component.has_reactive_assignments = true; - } + if (component.imported_declarations.has(name)) return; + if (scope.findOwner(name) !== component.instance_scope) return; + + pending_assignments.add(name); + component.has_reactive_assignments = true; } if (pending_assignments.size > 0) { diff --git a/src/compile/render-dom/wrappers/Element/index.ts b/src/compile/render-dom/wrappers/Element/index.ts index 35b73bc4f8..1178eca4ab 100644 --- a/src/compile/render-dom/wrappers/Element/index.ts +++ b/src/compile/render-dom/wrappers/Element/index.ts @@ -682,10 +682,7 @@ export default class ElementWrapper extends Wrapper { const params = this.node.animation.expression ? this.node.animation.expression.render() : '{}'; - let { name } = this.node.animation; - if (!component.hoistable_names.has(name) && !component.imported_declarations.has(name)) { - name = `ctx.${name}`; - } + const name = component.qualify(this.node.animation.name); block.builders.animate.addBlock(deindent` if (${animation}) ${animation}.stop(); diff --git a/test/runtime/samples/animation-js-easing/_config.js b/test/runtime/samples/animation-js-easing/_config.js new file mode 100644 index 0000000000..a31825c3f9 --- /dev/null +++ b/test/runtime/samples/animation-js-easing/_config.js @@ -0,0 +1,56 @@ +export default { + props: { + things: [ + { id: 1, name: 'a' }, + { id: 2, name: 'b' }, + { id: 3, name: 'c' }, + { id: 4, name: 'd' }, + { id: 5, name: 'e' } + ] + }, + + html: ` +
a
+
b
+
c
+
d
+
e
+ `, + + test({ assert, component, target, window, raf }) { + let divs = document.querySelectorAll('div'); + divs.forEach(div => { + div.getBoundingClientRect = function() { + const index = [...this.parentNode.children].indexOf(this); + const top = index * 30; + + return { + left: 0, + right: 100, + top, + bottom: top + 20 + } + }; + }) + + component.things = [ + { id: 5, name: 'e' }, + { id: 2, name: 'b' }, + { id: 3, name: 'c' }, + { id: 4, name: 'd' }, + { id: 1, name: 'a' } + ]; + + divs = document.querySelectorAll('div'); + assert.equal(divs[0].dy, 120); + assert.equal(divs[4].dy, -120); + + raf.tick(50); + assert.equal(divs[0].dy, 60); + assert.equal(divs[4].dy, -60); + + raf.tick(100); + assert.equal(divs[0].dy, 0); + assert.equal(divs[4].dy, 0); + } +}; \ No newline at end of file diff --git a/test/runtime/samples/animation-js-easing/easing.js b/test/runtime/samples/animation-js-easing/easing.js new file mode 100644 index 0000000000..fecc4d96f6 --- /dev/null +++ b/test/runtime/samples/animation-js-easing/easing.js @@ -0,0 +1,3 @@ +export function linear(t) { + return t; +} \ No newline at end of file diff --git a/test/runtime/samples/animation-js-easing/main.html b/test/runtime/samples/animation-js-easing/main.html new file mode 100644 index 0000000000..264ff6d9d6 --- /dev/null +++ b/test/runtime/samples/animation-js-easing/main.html @@ -0,0 +1,22 @@ + + +{#each things as thing (thing.id)} +
{thing.name}
+{/each} \ No newline at end of file