Skip to content

Commit

Permalink
fix: revert buggy reactive vars optimization (#8382)
Browse files Browse the repository at this point in the history
Reverts #7942
Fixes #8374
  • Loading branch information
dummdidumm committed Mar 15, 2023
1 parent a1e8421 commit 68e492e
Show file tree
Hide file tree
Showing 9 changed files with 40 additions and 101 deletions.
19 changes: 3 additions & 16 deletions src/compiler/compile/Component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ import compiler_warnings from './compiler_warnings';
import compiler_errors from './compiler_errors';
import { extract_ignores_above_position, extract_svelte_ignore_from_comments } from '../utils/extract_svelte_ignore';
import check_enable_sourcemap from './utils/check_enable_sourcemap';
import is_dynamic from './render_dom/wrappers/shared/is_dynamic';

interface ComponentOptions {
namespace?: string;
Expand Down Expand Up @@ -1392,11 +1391,12 @@ export default class Component {
module_dependencies.add(name);
}
}

const is_writable_or_mutated =
variable && (variable.writable || variable.mutated);
if (
should_add_as_dependency &&
(!owner || owner === component.instance_scope) &&
(name[0] === '$' || variable)
(name[0] === '$' || is_writable_or_mutated)
) {
dependencies.add(name);
}
Expand All @@ -1420,19 +1420,6 @@ export default class Component {
const { expression } = node.body as ExpressionStatement;
const declaration = expression && (expression as AssignmentExpression).left;

const is_dependency_static = Array.from(dependencies).every(
dependency => dependency !== '$$props' && dependency !== '$$restProps' && !is_dynamic(this.var_lookup.get(dependency))
);

if (is_dependency_static) {
assignees.forEach(assignee => {
const variable = component.var_lookup.get(assignee);
if (variable) {
variable.is_reactive_static = true;
}
});
}

unsorted_reactive_declarations.push({
assignees,
dependencies,
Expand Down
17 changes: 4 additions & 13 deletions src/compiler/compile/render_dom/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -390,13 +390,13 @@ export default function dom(
const resubscribable_reactive_store_unsubscribers = reactive_stores
.filter(store => {
const variable = component.var_lookup.get(store.name.slice(1));
return variable && (variable.reassigned || variable.export_name) && !variable.is_reactive_static;
return variable && (variable.reassigned || variable.export_name);
})
.map(({ name }) => b`$$self.$$.on_destroy.push(() => ${`$$unsubscribe_${name.slice(1)}`}());`);

if (has_definition) {
const reactive_declarations: Node[] = [];
const fixed_reactive_declarations: Array<Node | Node[]> = []; // not really 'reactive' but whatever
const reactive_declarations: (Node | Node[]) = [];
const fixed_reactive_declarations: Node[] = []; // not really 'reactive' but whatever

component.reactive_declarations.forEach(d => {
const dependencies = Array.from(d.dependencies);
Expand All @@ -417,15 +417,6 @@ export default function dom(
reactive_declarations.push(statement);
} else {
fixed_reactive_declarations.push(statement);
for (const assignee of d.assignees) {
const variable = component.var_lookup.get(assignee);
if (variable && variable.subscribable) {
fixed_reactive_declarations.push(b`
${component.compile_options.dev && b`@validate_store(${assignee}, '${assignee}');`}
@component_subscribe($$self, ${assignee}, $$value => $$invalidate(${renderer.context_lookup.get('$' + assignee).index}, ${'$' + assignee} = $$value));
`);
}
}
}
});

Expand All @@ -439,7 +430,7 @@ export default function dom(
const name = $name.slice(1);

const store = component.var_lookup.get(name);
if (store && (store.reassigned || store.export_name) && !store.is_reactive_static) {
if (store && (store.reassigned || store.export_name)) {
const unsubscribe = `$$unsubscribe_${name}`;
const subscribe = `$$subscribe_${name}`;
const i = renderer.context_lookup.get($name).index;
Expand Down
1 change: 0 additions & 1 deletion src/compiler/compile/render_dom/invalidate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ export function invalidate(renderer: Renderer, scope: Scope, node: Node, names:
!variable.hoistable &&
!variable.global &&
!variable.module &&
!variable.is_reactive_static &&
(
variable.referenced ||
variable.subscribable ||
Expand Down
1 change: 0 additions & 1 deletion src/compiler/interfaces.ts
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,6 @@ export interface Var {
subscribable?: boolean;
is_reactive_dependency?: boolean;
imported?: boolean;
is_reactive_static?: boolean;
}

export interface CssResult {
Expand Down
11 changes: 8 additions & 3 deletions test/js/samples/reactive-class-optimized/expected.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
noop,
safe_not_equal,
space,
subscribe,
toggle_class
} from "svelte/internal";

Expand Down Expand Up @@ -132,8 +133,13 @@ let reactiveModuleVar = Math.random();
function instance($$self, $$props, $$invalidate) {
let reactiveDeclaration;
let $reactiveStoreVal;
let $reactiveDeclaration;

let $reactiveDeclaration,
$$unsubscribe_reactiveDeclaration = noop,
$$subscribe_reactiveDeclaration = () => ($$unsubscribe_reactiveDeclaration(), $$unsubscribe_reactiveDeclaration = subscribe(reactiveDeclaration, $$value => $$invalidate(3, $reactiveDeclaration = $$value)), reactiveDeclaration);

component_subscribe($$self, reactiveStoreVal, $$value => $$invalidate(2, $reactiveStoreVal = $$value));
$$self.$$.on_destroy.push(() => $$unsubscribe_reactiveDeclaration());
nonReactiveGlobal = Math.random();
const reactiveConst = { x: Math.random() };
reactiveModuleVar += 1;
Expand All @@ -142,8 +148,7 @@ function instance($$self, $$props, $$invalidate) {
reactiveConst.x += 1;
}

$: reactiveDeclaration = reactiveModuleVar * 2;
component_subscribe($$self, reactiveDeclaration, $$value => $$invalidate(3, $reactiveDeclaration = $$value));
$: $$subscribe_reactiveDeclaration($$invalidate(1, reactiveDeclaration = reactiveModuleVar * 2));
return [reactiveConst, reactiveDeclaration, $reactiveStoreVal, $reactiveDeclaration];
}

Expand Down
60 changes: 0 additions & 60 deletions test/js/samples/reactive-values/expected.js

This file was deleted.

7 changes: 0 additions & 7 deletions test/js/samples/reactive-values/input.svelte

This file was deleted.

14 changes: 14 additions & 0 deletions test/runtime/samples/reactive-statement-indirect/_config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
export default {
html: `
<h1>2</h1>
<button>Increment</button>
`,
async test({ assert, target }) {
await target.querySelector('button').dispatchEvent(new window.MouseEvent('click'));

assert.htmlEqual(target.innerHTML, `
<h1>4</h1>
<button>Increment</button>
`);
}
};
11 changes: 11 additions & 0 deletions test/runtime/samples/reactive-statement-indirect/main.svelte
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<script>
let count = 1;
// Could be a let or simplified, but this tests that it still works like this
$: indirect_double = 2;
$: if (count > 0) {
indirect_double = count * 2;
}
</script>

<h1>{indirect_double}</h1>
<button on:click={() => count++}>Increment</button>

0 comments on commit 68e492e

Please sign in to comment.