From 6d67e018fecf3aa337402a183c81e1d0d6a6f3eb Mon Sep 17 00:00:00 2001 From: Richard Harris Date: Sat, 7 Sep 2019 22:55:17 -0400 Subject: [PATCH] inline $$invalidate calls - fixes #3512 --- src/compiler/compile/Component.ts | 2 +- .../compile/nodes/shared/Expression.ts | 150 ++++++++---------- src/compiler/compile/render_dom/index.ts | 127 ++++++--------- src/runtime/internal/Component.ts | 3 +- src/runtime/internal/utils.ts | 5 + .../expected.js | 2 +- .../invalidation-in-if-condition/_config.js | 18 +++ .../invalidation-in-if-condition/main.svelte | 12 ++ .../samples/store-resubscribe/_config.js | 2 +- 9 files changed, 155 insertions(+), 166 deletions(-) create mode 100644 test/runtime/samples/invalidation-in-if-condition/_config.js create mode 100644 test/runtime/samples/invalidation-in-if-condition/main.svelte diff --git a/src/compiler/compile/Component.ts b/src/compiler/compile/Component.ts index 68efd7f763..9f2a62fe93 100644 --- a/src/compiler/compile/Component.ts +++ b/src/compiler/compile/Component.ts @@ -813,7 +813,7 @@ export default class Component { const variable = this.var_lookup.get(name); if (variable && (variable.subscribable && variable.reassigned)) { - return `$$subscribe_${name}(), $$invalidate('${name}', ${value || name})`; + return `$$subscribe_${name}($$invalidate('${name}', ${value || name}))`; } if (name[0] === '$' && name[1] !== '$') { diff --git a/src/compiler/compile/nodes/shared/Expression.ts b/src/compiler/compile/nodes/shared/Expression.ts index d054416717..bb51c3b14b 100644 --- a/src/compiler/compile/nodes/shared/Expression.ts +++ b/src/compiler/compile/nodes/shared/Expression.ts @@ -10,10 +10,11 @@ import Wrapper from '../../render_dom/wrappers/shared/Wrapper'; import TemplateScope from './TemplateScope'; import get_object from '../../utils/get_object'; -import { nodes_match } from '../../../utils/nodes_match'; +// import { nodes_match } from '../../../utils/nodes_match'; import Block from '../../render_dom/Block'; import { INode } from '../interfaces'; import is_dynamic from '../../render_dom/wrappers/shared/is_dynamic'; +import { nodes_match } from '../../../utils/nodes_match'; const binary_operators: Record = { '**': 15, @@ -241,7 +242,6 @@ export default class Expression { const { code } = component; let function_expression; - let pending_assignments: Set = new Set(); let dependencies: Set; let contextual_dependencies: Set; @@ -309,16 +309,6 @@ export default class Expression { if (map.has(node)) scope = scope.parent; if (node === function_expression) { - if (pending_assignments.size > 0) { - if (node.type !== 'ArrowFunctionExpression') { - // this should never happen! - throw new Error(`Well that's odd`); - } - - // TOOD optimisation — if this is an event handler, - // the return value doesn't matter - } - const name = component.get_unique_name( sanitize(get_function_name(node, owner)) ); @@ -334,35 +324,10 @@ export default class Expression { args.push(original_params); } + // TODO is this still necessary? let body = code.slice(node.body.start, node.body.end).trim(); if (node.body.type !== 'BlockStatement') { - if (pending_assignments.size > 0) { - const dependencies = new Set(); - pending_assignments.forEach(name => { - if (template_scope.names.has(name)) { - template_scope.dependencies_for_name.get(name).forEach(dependency => { - dependencies.add(dependency); - }); - } else { - dependencies.add(name); - } - }); - - const insert = Array.from(dependencies).map(name => component.invalidate(name)).join('; '); - pending_assignments = new Set(); - - component.has_reactive_assignments = true; - - body = deindent` - { - const $$result = ${body}; - ${insert}; - return $$result; - } - `; - } else { - body = `{\n\treturn ${body};\n}`; - } + body = `{\n\treturn ${body};\n}`; } const fn = deindent` @@ -421,65 +386,78 @@ export default class Expression { contextual_dependencies = null; } - if (node.type === 'AssignmentExpression') { - const names = node.left.type === 'MemberExpression' - ? [get_object(node.left).name] - : extract_names(node.left); - - if (node.operator === '=' && nodes_match(node.left, node.right)) { - const dirty = names.filter(name => { - return !scope.declarations.has(name); - }); + // TODO dry out — most of this is shared with render_dom/index.ts + if (node.type === 'AssignmentExpression' || node.type === 'UpdateExpression') { + const assignee = node.type === 'AssignmentExpression' ? node.left : node.argument; - if (dirty.length) component.has_reactive_assignments = true; + // normally (`a = 1`, `b.c = 2`), there'll be a single name + // (a or b). In destructuring cases (`[d, e] = [e, d]`) there + // may be more, in which case we need to tack the extra ones + // onto the initial function call + const names = new Set(assignee.type === 'MemberExpression' + ? [get_object(assignee).name] + : extract_names(assignee)); - code.overwrite(node.start, node.end, dirty.map(n => component.invalidate(n)).join('; ')); - } else { - names.forEach(name => { - if (scope.declarations.has(name)) return; + const traced: Set = new Set(); + names.forEach(name => { + const dependencies = template_scope.dependencies_for_name.get(name); + if (dependencies) { + dependencies.forEach(name => traced.add(name)); + } else { + traced.add(name); + } + }); - const variable = component.var_lookup.get(name); - if (variable && variable.hoistable) return; + const [head, ...tail] = Array.from(traced).filter(name => { + const owner = scope.find_owner(name); + if (owner && owner !== component.instance_scope) return; + + const variable = component.var_lookup.get(name); + + return variable && ( + !variable.hoistable && + !variable.global && + !variable.module && + ( + variable.referenced || + variable.is_reactive_dependency || + variable.export_name + ) + ); + }); - pending_assignments.add(name); - }); - } - } else if (node.type === 'UpdateExpression') { - const { name } = get_object(node.argument); + if (head) { + component.has_reactive_assignments = true; - if (scope.declarations.has(name)) return; + if (node.operator === '=' && nodes_match(node.left, node.right) && tail.length === 0) { + code.overwrite(node.start, node.end, component.invalidate(head)); + } else { + let suffix = ')'; - const variable = component.var_lookup.get(name); - if (variable && variable.hoistable) return; + if (head[0] === '$') { + code.prependRight(node.start, `${component.helper('set_store_value')}(${head.slice(1)}, `); + } else { + let prefix = `$$invalidate`; - pending_assignments.add(name); - } + const variable = component.var_lookup.get(head); + if (variable.subscribable && variable.reassigned) { + prefix = `$$subscribe_${head}($$invalidate`; + suffix += `)`; + } - if (/Statement/.test(node.type)) { - if (pending_assignments.size > 0) { - const has_semi = code.original[node.end - 1] === ';'; + code.prependRight(node.start, `${prefix}('${head}', `); + } - const insert = ( - (has_semi ? ' ' : '; ') + - Array.from(pending_assignments).map(name => component.invalidate(name)).join('; ') - ); + const extra_args = tail.map(name => component.invalidate(name)); - if (/^(Break|Continue|Return)Statement/.test(node.type)) { - if (node.argument) { - code.overwrite(node.start, node.argument.start, `var $$result = `); - code.appendLeft(node.end, `${insert}; return $$result`); - } else { - code.prependRight(node.start, `${insert}; `); + if (assignee.type !== 'Identifier' || node.type === 'UpdateExpression' && !node.prefix || extra_args.length > 0) { + extra_args.unshift(head); } - } else if (parent && /(If|For(In|Of)?|While)Statement/.test(parent.type) && node.type !== 'BlockStatement') { - code.prependRight(node.start, '{ '); - code.appendLeft(node.end, `${insert}; }`); - } else { - code.appendLeft(node.end, `${insert};`); - } - component.has_reactive_assignments = true; - pending_assignments = new Set(); + suffix = `${extra_args.map(arg => `, ${arg}`).join('')}${suffix}`; + + code.appendLeft(node.end, suffix); + } } } } diff --git a/src/compiler/compile/render_dom/index.ts b/src/compiler/compile/render_dom/index.ts index 4895c97748..26e5d04311 100644 --- a/src/compiler/compile/render_dom/index.ts +++ b/src/compiler/compile/render_dom/index.ts @@ -8,8 +8,9 @@ import { walk } from 'estree-walker'; import { stringify_props } from '../utils/stringify_props'; import add_to_set from '../utils/add_to_set'; import get_object from '../utils/get_object'; -import { extract_names } from '../utils/scope'; +// import { extract_names } from '../utils/scope'; import { nodes_match } from '../../utils/nodes_match'; +import { extract_names } from '../utils/scope'; export default function dom( component: Component, @@ -158,8 +159,6 @@ export default function dom( let scope = component.instance_scope; const map = component.instance_scope_map; - let pending_assignments = new Set(); - walk(component.ast.instance.content, { enter: (node) => { if (map.has(node)) { @@ -167,102 +166,78 @@ export default function dom( } }, - leave(node, parent) { + leave(node) { if (map.has(node)) { scope = scope.parent; } + // TODO dry out — most of this is shared with Expression.ts if (node.type === 'AssignmentExpression' || node.type === 'UpdateExpression') { const assignee = node.type === 'AssignmentExpression' ? node.left : node.argument; - let names = []; - - if (assignee.type === 'MemberExpression') { - const left_object_name = get_object(assignee).name; - left_object_name && (names = [left_object_name]); - } else { - names = extract_names(assignee); - } - - if (node.operator === '=' && nodes_match(node.left, node.right)) { - const dirty = names.filter(name => { - return name[0] === '$' || scope.find_owner(name) === component.instance_scope; - }); - if (dirty.length) component.has_reactive_assignments = true; - - code.overwrite(node.start, node.end, dirty.map(n => component.invalidate(n)).join('; ')); - } else { - const single = ( - node.type === 'AssignmentExpression' && - assignee.type === 'Identifier' && - parent.type === 'ExpressionStatement' && - assignee.name[0] !== '$' + // normally (`a = 1`, `b.c = 2`), there'll be a single name + // (a or b). In destructuring cases (`[d, e] = [e, d]`) there + // may be more, in which case we need to tack the extra ones + // onto the initial function call + const names = new Set(assignee.type === 'MemberExpression' + ? [get_object(assignee).name] + : extract_names(assignee)); + + const [head, ...tail] = Array.from(names).filter(name => { + const owner = scope.find_owner(name); + if (owner && owner !== component.instance_scope) return; + + const variable = component.var_lookup.get(name); + + return variable && ( + !variable.hoistable && + !variable.global && + !variable.module && + ( + variable.referenced || + variable.is_reactive_dependency || + variable.export_name + ) ); + }); - names.forEach(name => { - const owner = scope.find_owner(name); - if (owner && owner !== component.instance_scope) return; + if (head) { + component.has_reactive_assignments = true; - const variable = component.var_lookup.get(name); - if (variable && (variable.hoistable || variable.global || variable.module)) return; + if (node.operator === '=' && nodes_match(node.left, node.right) && tail.length === 0) { + code.overwrite(node.start, node.end, component.invalidate(head)); + } else { + let suffix = ')'; - if (single && !(variable.subscribable && variable.reassigned)) { - if (variable.referenced || variable.is_reactive_dependency || variable.export_name) { - code.prependRight(node.start, `$$invalidate('${name}', `); - code.appendLeft(node.end, `)`); - } + if (head[0] === '$') { + code.prependRight(node.start, `${component.helper('set_store_value')}(${head.slice(1)}, `); } else { - pending_assignments.add(name); - } + let prefix = `$$invalidate`; - component.has_reactive_assignments = true; - }); - } - } - - if (pending_assignments.size > 0) { - if (node.type === 'ArrowFunctionExpression') { - const insert = Array.from(pending_assignments).map(name => component.invalidate(name)).join('; '); - pending_assignments = new Set(); - - code.prependRight(node.body.start, `{ const $$result = `); - code.appendLeft(node.body.end, `; ${insert}; return $$result; }`); + const variable = component.var_lookup.get(head); + if (variable.subscribable && variable.reassigned) { + prefix = `$$subscribe_${head}($$invalidate`; + suffix += `)`; + } - pending_assignments = new Set(); - } + code.prependRight(node.start, `${prefix}('${head}', `); + } - else if (/Statement/.test(node.type)) { - const insert = Array.from(pending_assignments).map(name => component.invalidate(name)).join('; '); + const extra_args = tail.map(name => component.invalidate(name)); - if (/^(Break|Continue|Return)Statement/.test(node.type)) { - if (node.argument) { - code.overwrite(node.start, node.argument.start, `var $$result = `); - code.appendLeft(node.argument.end, `; ${insert}; return $$result`); - } else { - code.prependRight(node.start, `${insert}; `); + if (assignee.type !== 'Identifier' || node.type === 'UpdateExpression' && !node.prefix || extra_args.length > 0) { + extra_args.unshift(head); } - } else if (parent && /(If|For(In|Of)?|While)Statement/.test(parent.type) && node.type !== 'BlockStatement') { - code.prependRight(node.start, '{ '); - code.appendLeft(node.end, `${code.original[node.end - 1] === ';' ? '' : ';'} ${insert}; }`); - } else { - code.appendLeft(node.end, `${code.original[node.end - 1] === ';' ? '' : ';'} ${insert};`); - } - pending_assignments = new Set(); - } else if (parent && parent.type !== 'ForStatement' && node.type === 'VariableDeclaration') { - const insert = Array.from(pending_assignments).map(name => component.invalidate(name)).join('; '); + suffix = `${extra_args.map(arg => `, ${arg}`).join('')}${suffix}`; - code.appendLeft(node.end, `${code.original[node.end - 1] === ';' ? '' : ';'} ${insert};`); - pending_assignments = new Set(); + code.appendLeft(node.end, suffix); + } } } } }); - if (pending_assignments.size > 0) { - throw new Error(`TODO this should not happen!`); - } - component.rewrite_props(({ name, reassigned }) => { const value = `$${name}`; @@ -395,7 +370,7 @@ export default function dom( const store = component.var_lookup.get(name); if (store && store.reassigned) { - return `${$name}, $$unsubscribe_${name} = @noop, $$subscribe_${name} = () => { $$unsubscribe_${name}(); $$unsubscribe_${name} = @subscribe(${name}, $$value => { ${$name} = $$value; $$invalidate('${$name}', ${$name}); }) }`; + return `${$name}, $$unsubscribe_${name} = @noop, $$subscribe_${name} = () => ($$unsubscribe_${name}(), $$unsubscribe_${name} = @subscribe(${name}, $$value => { ${$name} = $$value; $$invalidate('${$name}', ${$name}); }), ${name})`; } return $name; diff --git a/src/runtime/internal/Component.ts b/src/runtime/internal/Component.ts index 92e227e57c..d5b4f83bee 100644 --- a/src/runtime/internal/Component.ts +++ b/src/runtime/internal/Component.ts @@ -101,11 +101,12 @@ export function init(component, options, instance, create_fragment, not_equal, p let ready = false; $$.ctx = instance - ? instance(component, props, (key, value) => { + ? instance(component, props, (key, ret, value = ret) => { if ($$.ctx && not_equal($$.ctx[key], $$.ctx[key] = value)) { if ($$.bound[key]) $$.bound[key](value); if (ready) make_dirty(component, key); } + return ret; }) : props; diff --git a/src/runtime/internal/utils.ts b/src/runtime/internal/utils.ts index e457eb0505..d267b73cee 100644 --- a/src/runtime/internal/utils.ts +++ b/src/runtime/internal/utils.ts @@ -100,3 +100,8 @@ export function once(fn) { export function null_to_empty(value) { return value == null ? '' : value; } + +export function set_store_value(store, ret, value = ret) { + store.set(value); + return ret; +} diff --git a/test/js/samples/instrumentation-template-if-no-block/expected.js b/test/js/samples/instrumentation-template-if-no-block/expected.js index 1b8cbb257a..d24aeabf7f 100644 --- a/test/js/samples/instrumentation-template-if-no-block/expected.js +++ b/test/js/samples/instrumentation-template-if-no-block/expected.js @@ -61,7 +61,7 @@ function instance($$self, $$props, $$invalidate) { let x = 0; function click_handler() { - if (true) { x += 1; $$invalidate('x', x); } + if (true) $$invalidate('x', x += 1); } return { x, click_handler }; diff --git a/test/runtime/samples/invalidation-in-if-condition/_config.js b/test/runtime/samples/invalidation-in-if-condition/_config.js new file mode 100644 index 0000000000..60b02d9934 --- /dev/null +++ b/test/runtime/samples/invalidation-in-if-condition/_config.js @@ -0,0 +1,18 @@ +export default { + show: 1, + html: ``, + + async test({ assert, target, window }) { + const button = target.querySelector('button'); + const click = new window.MouseEvent('click'); + + await button.dispatchEvent(click); + assert.htmlEqual(target.innerHTML, ``); + + await button.dispatchEvent(click); + assert.htmlEqual(target.innerHTML, ``); + + await button.dispatchEvent(click); + assert.htmlEqual(target.innerHTML, ``); + } +}; \ No newline at end of file diff --git a/test/runtime/samples/invalidation-in-if-condition/main.svelte b/test/runtime/samples/invalidation-in-if-condition/main.svelte new file mode 100644 index 0000000000..810576695e --- /dev/null +++ b/test/runtime/samples/invalidation-in-if-condition/main.svelte @@ -0,0 +1,12 @@ + + + \ No newline at end of file diff --git a/test/runtime/samples/store-resubscribe/_config.js b/test/runtime/samples/store-resubscribe/_config.js index fed8caa805..6bde764a55 100644 --- a/test/runtime/samples/store-resubscribe/_config.js +++ b/test/runtime/samples/store-resubscribe/_config.js @@ -5,7 +5,7 @@ export default { `, - async test({ assert, component, target, window }) { + async test({ assert, target, window }) { const buttons = target.querySelectorAll('button'); const click = new window.MouseEvent('click');