fix: hoist reactive imports to the module

[paoloricciuti]

Svelte 5 rewrite

Closes #12842

Reactive imports should be hoisted to the module in case someone reads or write to them in the module context. I've check and it seems the only thing that is doing is create a source so it should be good to just hoist them to module.

Please note that the Svelte codebase is currently being rewritten for Svelte 5. Changes should target Svelte 5, which lives on the default branch (main).

If your PR concerns Svelte 4 (including updates to svelte.dev.docs), please ensure the base branch is svelte-4 and not main.

Before submitting the PR, please make sure you do the following

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• Prefix your PR title with feat:, fix:, chore:, or docs:.
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests and linting

• Run the tests with pnpm test and lint the project with pnpm lint

[changeset-bot]

🦋 Changeset detected

Latest commit: 6e79526

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
svelte	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[Rich-Harris]

Not sure this is quite right. In Svelte 4, mutating a module-level declaration has no effect. I think we just need to skip the legacy_reactive_import transformation entirely for these imports

[paoloricciuti]

Not sure this is quite right. In Svelte 4, mutating a module-level declaration has no effect. I think we just need to skip the legacy_reactive_import transformation entirely for these imports

Ohhh i actually never used this feature so i wasn't sure...fixing it.

[paoloricciuti]

Not sure this is quite right. In Svelte 4, mutating a module-level declaration has no effect. I think we just need to skip the legacy_reactive_import transformation entirely for these imports

I tought i could've just used is_instance to check but the actual fix is much more involved for the following reasons:

1. the code that populate legacy_reactive_imports is in the Program visitor but just loop over every import declaration in the scope the first time around so is_instance is actually always false
2. So to know if the import is in the module or in the instance i had to add a metadata property to the binding (i have a vague memory that we wanted to get rid of metadata right? Should i put it somewhere else)
3. Now i can avoid create the legacy_reactive_imports for the imports in the module....BUT...when the import is in the instance i need to create them and only apply the transform to the mutate/read that are actually in the instance and not the ones in module...this however leads to...
4. there's currently no way to specify that a transform should selectively apply, the only selective application is around the declaration itself. So i started adding a should_transform function to the transform object that should take the whole context as input to allow each transform to decide if it should apply or not. But is still not over because
5. the functions that use the transformers sometimes doesn't have access to the whole context but just the state and the state might not be enough to actually determine if the transformer should apply or not (for example the ClientTransformState in build_getter doesn't even have is_instance in the type because it could be called from the template where is_instance is not there) so we should probably just accept ClientTransformState and check for is_instance===false to apply the transform in this case? But what is in the future we need more sofistication to apply the transform?

I will continue working on this a bit but since is touching a lot of places i would love some feedback on this...maybe i'm missing something obvious.

[Rich-Harris]

Fixed. The trick is to compare the start and end with the instance script

[paoloricciuti]

Fixed. The trick is to compare the start and end with the instance script

Uh I'm always a bit hesitant to use text positions but I guess it makes sense...noted for next time.

However this doesn't fully fix the problem. If you do something like this it stills throws an error.

[paoloricciuti]

Oh also you didn't revert my initial change

[Rich-Harris]

The initial change was fine — there's no need to create a store per component when we can have one per import instead.

If you do something like this it stills throws an error.

Huh. To be honest the Svelte 4 behaviour makes no sense here — given that it was untested, and that this is code that no-one would ever write, I think this is a place where we don't need to worry about bug-for-bug compatibility, as long as we preserve the intended behaviour for realistic cases.

[paoloricciuti]

Great then...yeah i was trying to make it same behavior of svelte 4 in toto but I agree it doesn't make much sense.

Then I guess is fine like this