Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Svelte 5: desync in variable imported into components #12489

Closed
TehBrian opened this issue Jul 18, 2024 · 5 comments · Fixed by #12493
Closed

Svelte 5: desync in variable imported into components #12489

TehBrian opened this issue Jul 18, 2024 · 5 comments · Fixed by #12493
Assignees
Labels
Milestone

Comments

@TehBrian
Copy link

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
Copy link

webJose commented Jul 18, 2024

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
Copy link
Contributor

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 dummdidumm added this to the 5.0 milestone Jul 18, 2024
@dummdidumm dummdidumm added the bug label Jul 18, 2024
@dummdidumm
Copy link
Member

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

@TehBrian
Copy link
Author

TehBrian commented Jul 18, 2024

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
Copy link
Contributor

Prinzhorn commented Jul 18, 2024

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants