warn module variables are nonreactive and make them truly nonreactive (…

…sveltejs#5847)

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 ## Unreleased
 
+* Warn when using `module` variables reactively, and close weird reactivity loophole ([#5847](https://github.com/sveltejs/svelte/pull/5847))
 * Throw a parser error for `class:` directives with an empty class name ([#5858](https://github.com/sveltejs/svelte/issues/5858))
 * Fix extraneous store subscription in SSR mode ([#5883](https://github.com/sveltejs/svelte/issues/5883))
 * Don't emit update code for `class:` directives whose expression is not dynamic ([#5919](https://github.com/sveltejs/svelte/issues/5919))

src/compiler/compile/Component.ts

@@ -1175,15 +1175,20 @@ export default class Component {
  extract_reactive_declarations() {
  const component = this;
 
- const unsorted_reactive_declarations = [];
+ const unsorted_reactive_declarations: Array<{
+ assignees: Set<string>;
+ dependencies: Set<string>;
+ node: Node;
+ declaration: Node;
+ }> = [];
 
  this.ast.instance.content.body.forEach(node => {
  if (node.type === 'LabeledStatement' && node.label.name === '$') {
  this.reactive_declaration_nodes.add(node);
 
- const assignees = new Set();
+ const assignees = new Set<string>();
  const assignee_nodes = new Set();
- const dependencies = new Set();
+ const dependencies = new Set<string>();
 
  let scope = this.instance_scope;
  const map = this.instance_scope_map;
@@ -1214,10 +1219,22 @@ export default class Component {
  const { name } = identifier;
  const owner = scope.find_owner(name);
  const variable = component.var_lookup.get(name);
- if (variable) variable.is_reactive_dependency = true;
+ let should_add_as_dependency = true;
+
+ if (variable) {
+ variable.is_reactive_dependency = true;
+ if (variable.module) {
+ should_add_as_dependency = false;
+ component.warn(node as any, {
+ code: 'module-script-reactive-declaration',
+ message: `"${name}" is declared in a module script and will not be reactive`
+ });
+ }
+ }
  const is_writable_or_mutated =
  variable && (variable.writable || variable.mutated);
  if (
+ should_add_as_dependency &&
  (!owner || owner === component.instance_scope) &&
  (name[0] === '$' || is_writable_or_mutated)
  ) {

test/js/samples/reactive-class-optimized/expected.js

@@ -148,12 +148,7 @@ function instance($$self, $$props, $$invalidate) {
  reactiveConst.x += 1;
  }
 
- $$self.$$.update = () => {
- if ($$self.$$.dirty & /*reactiveModuleVar*/ 0) {
- $: $$subscribe_reactiveDeclaration($$invalidate(1, reactiveDeclaration = reactiveModuleVar * 2));
- }
- };
-
+ $: $$subscribe_reactiveDeclaration($$invalidate(1, reactiveDeclaration = reactiveModuleVar * 2));
  return [reactiveConst, reactiveDeclaration, $reactiveStoreVal, $reactiveDeclaration];
 }
 

test/runtime/samples/reactive-statement-module-vars/_config.js

@@ -0,0 +1,18 @@
+export default {
+ html: `
+ a: moduleA
+ b: moduleB
+ moduleA: moduleA
+ moduleB: moduleB
+ `,
+ async test({ assert, target, component }) {
+ await component.updateModuleA();
+
+ assert.htmlEqual(target.innerHTML, `
+ a: moduleA
+ b: moduleB
+ moduleA: moduleA
+ moduleB: moduleB
+ `);
+ }
+};

test/runtime/samples/reactive-statement-module-vars/main.svelte

@@ -0,0 +1,15 @@
+<script context="module">
+ let moduleA = 'moduleA';
+ let moduleB = 'moduleB';
+</script>
+<script>
+ export function updateModuleA() {
+ moduleA = 'something else';
+ }
+ $: a = moduleA;
+ $: b = moduleB;
+</script>
+a: {a}
+b: {b}
+moduleA: {moduleA}
+moduleB: {moduleB}

test/validator/samples/reactive-module-variable/input.svelte

@@ -0,0 +1,6 @@
+<script context="module">
+ let foo;
+</script>
+<script>
+ $: bar = foo;
+</script>

test/validator/samples/reactive-module-variable/warnings.json

@@ -0,0 +1,17 @@
+[
+ {
+ "code": "module-script-reactive-declaration",
+ "message": "\"foo\" is declared in a module script and will not be reactive",
+ "pos": 65,
+ "start": {
+ "character": 65,
+ "column": 10,
+ "line": 5
+ },
+ "end": {
+ "character": 68,
+ "column": 13,
+ "line": 5
+ }
+ }
+]