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: access last safe value of prop on unmount #15342

Closed
wants to merge 4 commits into from

Conversation

paoloricciuti
Copy link
Member

Closes #14725

Draft for the moment since i need to write proper tests for this but this seems to cover most cases. Basically when there's a situation that could lead to a component being unmounted (if or dynamic component) we inject a safe_props function call that proxifies the identifiers that are passed to components creating a derived for each accessed value and storing the old value in a map. We also register a teardown that set an unmounting flag to true so that we know when we are unmounting...in that case the derived returns the old and safe value instead.

There might be some other cases I'm not considering...i've successfully tested it with this if you want to try and break it.

<script>
	import Component from "./Component.svelte";
	
	let checked = $state(true);
	let count = $state(0);
	let spread = $derived({checked,count});

	let Dynamic = $derived(count < 5 ? Component : undefined);
	let Dynamic2 = $derived(checked ? Component : undefined);
</script>

<button onclick={()=>count++}>+1</button>
<button onclick={()=>checked = !checked}>flip</button>

{#if count < 5}
	<Component {count} bind:checked {...spread} />
{/if}

{#if checked}
	<Component {count} {checked} />
{/if}

<Dynamic {count} {checked} />

<Dynamic2 {count} {checked} />

with Component.svelte being

<script>
	let { checked, count } = $props();
	$effect(() => {
		return ()=>{
			console.log("component unmounting", count, checked);
		}
	});
</script>

{count}

we should also check this doesn't add too much overhead.

P.s. another possible side effect is that to store the old value initially we read every prop in untrack...this should be fine since they should all be identifiers but if they are getters users might get an extra undetected call.

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.
  • If this PR changes code within packages/svelte/src, add a changeset (npx changeset).

Tests and linting

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

Copy link
Contributor

Playground

pnpm add https://pkg.pr.new/svelte@15342

Copy link

changeset-bot bot commented Feb 20, 2025

🦋 Changeset detected

Latest commit: c2c2edd

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

Unfortunately this approach just doesn't work. It will only provide the latest value that was actually read — if a prop isn't read until teardown, or is only read intermittently, then it will be incorrect.

We would need to proactively use effects to update the latest values, which seems a bit undesirable. Anyway, I made a start on that approach in #15366

@paoloricciuti
Copy link
Member Author

Unfortunately this approach just doesn't work. It will only provide the latest value that was actually read — if a prop isn't read until teardown, or is only read intermittently, then it will be incorrect.

Mh yeah you are right...well, let's close this then

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.

Prop value can be different than guaranted by TypeScript
2 participants