sveltejs / svelte

Cybernetically enhanced web apps
https://svelte.dev
MIT License
78.18k stars 4.09k forks source link

Svelte 5: desync in variable imported into components #12489

Closed TehBrian closed 1 month ago

TehBrian commented 1 month ago

Describe the bug

I don't have much experience with Svelte, so please pardon any inaccuracies in this report.

A variable held in a script, imported into two components (parent and child), mutated by a function in the script, which is called by a function in the parent component, which is passed as a prop into the child component and called by another function, will have a different value depending on whether it is the parent component or the child component that accesses it.

The REPL showcases the issue quite concisely. Click the button multiple times, and look at the console. The numbers printed should be the same each click, but num in Component.svelte doesn't update from its initial state of 0.

I was discussing this with cirilla (gtmnayan) on the Svelte Discord (link to message), and they suspect "[t]hat's a bug with how event delegation is implemented."

Reproduction

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAE51RQU7DMBD8ymIhtRVRAtc0rYT6BI6EA7gb5BLvWraDQJb_jt20SQ8IBCfHk9nZmXEQnerRifoxCHrWKGrx4Nli6d6x9ygK4T9NRg8ufTserMw3_DBsPfTogQYNG7hdt9TSCe4Gkl4xgSJpUSP55QpCIvhMvtnAXWLHpKd5rzqFe1F7O2AsJg_3xsyrJyvT-sZJq4zfZkmljztDNlLMGyFCZ1nDoqwuA5UHt1hfjO04nZT5Z_aEnCaO9DQwheqYz3GSzJxwPSKSyXGPZc-vy2RphGNLTTWbpmbeG5JehGr7Ux0T-z-l_NZEfsSQUyXiBq6NZePGNJepmWSv5Nuc_FjDnzK_DN4npXCSittdPkDjFdSrphp_f9PDU_wCkWj2IKYCAAA=

Logs

No response

System Info

System:
    OS: macOS 14.5
    CPU: (8) arm64 Apple M1 Pro
    Memory: 2.30 GB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 22.4.0 - /opt/homebrew/bin/node
    npm: 10.8.1 - /opt/homebrew/bin/npm
  Browsers:
    Chrome: 126.0.6478.128
    Edge: 120.0.2210.144

Severity

annoyance

webJose commented 1 month ago

This is not a bug, as far as I can tell. An exported variable from a module is never a good thing because JS engines (primarily V8) cache (unsure if "cache" is the right word, but still) the exported objects. So even if num has been declared with let, exporting it will simply export its initial value. It doesn't export a reference to the variable, it exports a copy of the variable contents.

Basically, this is how JavaScript works.

Prinzhorn commented 1 month ago

Basically, this is how JavaScript works.

I disagree, this does not explain the behavior

they suspect "[t]hat's a bug with how event delegation is implemented."

Looks like it, took me a while to figure it out. This is (part of) the generated code for Component.svelte:

import * as $ from "svelte/internal/client";
import { num } from './Store.svelte.js';

function onclick(_, $$props, num) {
    $$props.foo();
    console.log(num);
}

Do you see it? The num argument of onclick is masking the num import. They should not have the same name.

But regardless, the pattern you are using is definitely not the way to go. You shouldn't mutate an exported variable like that. You could use $state:

dummdidumm commented 1 month ago

Bug in event delegation; num should not be part of the hoisted parameters

TehBrian commented 1 month ago

Thank you all for the incredible response time. The morning after (for me), and there's already a fix PR. I greatly appreciate this community and its framework.

But regardless, the pattern you are using is definitely not the way to go. You shouldn't mutate an exported variable like that. You could use $state:

Indirection (using either a container (object) or a getter, hiding direct access to the variable) was my first attempt at "fixing" the problem (not the root bug, but the symptom), and that did indeed work. Notice that using an object, even without $state(), also "fixes" the problem.

But it seems, to me, like a "gotcha" to allow components to import variables, and allow those variables to be reassigned by the importee, and allow those changes to be propagated to the importers, but then not allow those changes to be reflected across all importers (depending on the calling context). To me, if it's "bad practice" or "not guaranteed to work" to rely on an imported variable maintaining state after mutations, then all imported variables ought to be constant, as that would be most self-consistent.

I'm not an experienced front-end dev whatsoever, but those are just my two cents. If you have any counterpoints to my thoughts, I would love to hear them. :-)

Prinzhorn commented 1 month ago

But it seems, to me, like a "gotcha" to allow components to import variables, and allow those variables to be reassigned by the importee, and allow those changes to be propagated to the importers, but then not allow those changes to be reflected across all importers (depending on the calling context). To me, if it's "bad practice" or "not guaranteed to work" to rely on an imported variable maintaining state after mutations, then all imported variables ought to be constant, as that would be most self-consistent.

I was referring to what your code does in the context of Svelte (not to imports in general). Actively pulling num is not something you would do in actual code and I don't know what you are trying to achieve with it. If you make num a $state you don't need to "pull" it, it will magically and reactively update itself. This is the same REPL I posted above but it also outputs {num.value} (second version with automatic logging). That's not possible with a plain import (which you are effectively using like a global variable, which is bad practice).