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

fix: make beforeUpdate/afterUpdate behave more like Svelte 4 #10408

Merged
merged 28 commits into from
Feb 7, 2024

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Feb 6, 2024

Closes #9420.

As shown in #9442, beforeUpdate and afterUpdate aren't quite right at the moment — they will only fire when locally declared signals, or signals that are used in the template, change.

Unfortunately #9442 doesn't fix it, because it's quite possible for an afterUpdate callback to not reference any state. Turning it into an effect therefore doesn't solve the problem.

This PR proposes a solution that seems a bit simpler than the alternatives — we simply create an $effect.pre (before beforeUpdate and an $effect (for afterUpdate) and, inside those, listen for all locally declared signals plus reactive props.

This does mean that we need to link the locally declared signals to the component context (the reverse of the current behaviour, wherein we link the component context to locally declared signals). If we're averse to that, then one option would be to simply forbid the use of beforeUpdate/afterUpdate/onMount in a runes-mode component. This is probably a good thing to do anyway, since these functions are fundamentally unreliable in a runes-mode context (for example callbacks won't fire if a component imports some reactive state which isn't referenced inside the callback) and not recommended.

Notes:

  • this undoes a breaking change around re-running beforeUpdate callbacks twice on component initialisation
  • this adds a breaking change — parent afterUpdate callbacks now run after child afterUpdate callbacks. (Happily, this makes way more sense, and probably won't cause any problems in real usage)

Todo:

  • bind deriveds (and anything else?) to the component context
  • handle late-created signals (i.e. a signal that is created after the beforeUpdate callback runs)

Svelte 5 rewrite

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

Copy link

changeset-bot bot commented Feb 6, 2024

🦋 Changeset detected

Latest commit: 179ab89

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

One bit I don't love: we need to associate a dummy signal with the component context so that late-declared signals still cause callbacks to re-run. By that I mean that here...

beforeUpdate(() => {
  console.log('hello');
});

let x = $state(0);

...the callback would not re-run when x changed if the underlying effect only read the signals that were already associated with the component context.

Because of this, we need to add a skip flag that prevents the beforeUpdate callback from running multiple times. Because of that, we can't undo the breaking change that had been removed up until a couple of commits ago.

A nicer approach might be to defer the pre_effect until after the component has initialised. In other words, this...

<script>
	import { beforeUpdate } from 'svelte';

	beforeUpdate(() => {
		console.log('hello');
	});

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

<button on:click={() => count += 1}>clicks: {count}</button>

...would yield something like this...

export default function App($$anchor, $$props) {
	$.push($$props, true);

	beforeUpdate(() => {
		console.log('hello');
	});

	let count = $.source(0);

+	$.init();

	/* Init */
	var button = $.open($$anchor, true, frag);
	var text = $.child(button);

	/* Update */
	$.text_effect(text, () => `clicks: ${$.stringify($.get(count))}`);
	$.event("click", button, () => $.set(count, $.get(count) + 1), false);
	$.close($$anchor, button);
	$.pop();
}

...where $.init would create the pre_effect, safe in the knowledge that all relevant sources would already have been declared.

But it's annoying to add that code for the majority of components that don't use beforeUpdate. So my inclination would be to only do this if we decide that we're going to ban beforeUpdate and afterUpdate from runes-mode components.

Thoughts?

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great solution to this tricky problem!

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.

Svelte 5: afterUpdate only tracks props that are being used in the template
2 participants