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

feat: add $effect.root rune #9638

Merged
merged 4 commits into from Nov 27, 2023
Merged

feat: add $effect.root rune #9638

merged 4 commits into from Nov 27, 2023

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Nov 24, 2023

The $effect.root rune is an advanced feature that creates a non-tracked scope that doesn't auto-cleanup. This is useful for nested effects that you want to manually control. This rune also allows for creation of effects outside of the component initialisation phase.

<script>
	let count = $state(0);

	const cleanup = $effect.root(() => {
		$effect(() => {
			console.log(count);
		});

		return () => {
			console.log('effect root cleanup');
		};
	});
</script>

Copy link

changeset-bot bot commented Nov 24, 2023

🦋 Changeset detected

Latest commit: f08a4cc

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

Copy link

vercel bot commented Nov 24, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
svelte-5-preview ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 27, 2023 3:55pm

feat: add $effect.root rune

update doc

update doc

fix validation
@michael
Copy link

michael commented Nov 27, 2023

Could $effect itself return a reference to a cleanup function too? Instead of the runtime error I saw, the Svelte compiler could show a warning when $effect is used outside a component and the return value of $effect isn't assigned.

class Counter {
  count = $state(0);

  constructor(count) {
    this.count = count;
    this.cleanupCountLogger = $effect(() => { console.log(this.count); });
  }

  dispose() {
    this.cleanupCountLogger();
  }
}

Just an idea, but I understand if that's not explicit enough.

In any case, I'd not expect that an $effect is auto-cleaned up when used within a class, as the class instance's lifecycle is likely completely unrelated to the lifecycle of the components. So to me the restriction that $effect may only be called within the boundary of component initialization seems a bit arbitrary. Better be explicit about cleanup when $effect is used in code outside of components?

@trueadm
Copy link
Contributor Author

trueadm commented Nov 27, 2023

@michael I don't think we want to do that, as it'll be difficult to know if people are using effects properly and are disposing of them. This API is intended to be an advanced feature, so if people use it, they should expect to go out of the way to clean up their memory properly too. In the naive case, I just don't see how we can guarantee that if we make everything return a cleanup expression.

@trueadm
Copy link
Contributor Author

trueadm commented Nov 27, 2023

I've updated the PR to remove the 2nd argument from $effect.root.

@michael
Copy link

michael commented Nov 27, 2023

Yeah would definitely need more self-responsibility. And it's a good thing $effect does auto-cleanup inside components.

The solution with $effect.root is fine I guess, just leaves a bit of ambiguity, like users will likely use $effect outside of components when they're not really designed for that primarily. And then occasionally run into the problem I had (ERR_SVELTE_ORPHAN_EFFECT), and switch to $effect.root.

As for myself I will either go for $effect.root (once it's released) or only use $effect from the component so it has a clear lifecycle.

Just one question: When using $derived as class fields, I don't have to worry about this right?

@trueadm
Copy link
Contributor Author

trueadm commented Nov 27, 2023

@michael Derived is fine. We might also add an API to make it easier to understand if you're inside a parent component effect.

@Rich-Harris Rich-Harris merged commit 81d3e47 into main Nov 27, 2023
9 checks passed
@Rich-Harris Rich-Harris deleted the effect-root-rune branch November 27, 2023 18:34
@intrnl
Copy link

intrnl commented Dec 6, 2023

Seems like this PR closes #9269, which is great!

One question though, I suppose the reason why cleanup isn't being passed to the root fn is because it can be potentially confusing? I'd think makes $effect.root rather nice to work with

// Let's have a persistent counter
const counter = $effect.root((cleanup) => {
  let count = $state(0);

  $effect(() => {
    // persist the count
  });

  return {
    get count () { return count; },
    set count (next) { count = next; },

    // ... additional methods ...

    cleanup,
  };
});

counter.count = 8;

// We don't need it anymore, goodbye!
counter.cleanup();

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

Successfully merging this pull request may close these issues.

None yet

4 participants