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 (alternative) #15366

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/tasty-pears-travel.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

fix: access last safe value of prop on unmount
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ export function client_component(analysis, options) {
in_constructor: false,
instance_level_snippets: [],
module_level_snippets: [],
needs_safe_props: false,

// these are set inside the `Fragment` visitor, and cannot be used until then
init: /** @type {any} */ (null),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ export interface ComponentClientTransformState extends ClientTransformState {
readonly hoisted: Array<Statement | ModuleDeclaration>;
readonly events: Set<string>;
readonly is_instance: boolean;
readonly needs_safe_props: boolean;
readonly store_to_invalidate?: string;

/** Stuff that happens before the render effect(s) */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export function AwaitBlock(node, context) {
if (node.then) {
const then_context = {
...context,
state: { ...context.state, transform: { ...context.state.transform } }
state: { ...context.state, transform: { ...context.state.transform }, needs_safe_props: true }
};
const argument = node.value && create_derived_block_argument(node.value, then_context);

Expand All @@ -37,7 +37,7 @@ export function AwaitBlock(node, context) {
}

if (node.catch) {
const catch_context = { ...context, state: { ...context.state } };
const catch_context = { ...context, state: { ...context.state, needs_safe_props: true } };
const argument = node.error && create_derived_block_argument(node.error, catch_context);

/** @type {Pattern[]} */
Expand All @@ -59,7 +59,12 @@ export function AwaitBlock(node, context) {
context.state.node,
expression,
node.pending
? b.arrow([b.id('$$anchor')], /** @type {BlockStatement} */ (context.visit(node.pending)))
? b.arrow(
[b.id('$$anchor')],
/** @type {BlockStatement} */ (
context.visit(node.pending, { ...context.state, needs_safe_props: true })
)
)
: b.literal(null),
then_block,
catch_block
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,18 @@ import { build_component } from './shared/component.js';
export function Component(node, context) {
if (node.metadata.dynamic) {
// Handle dynamic references to what seems like static inline components
const component = build_component(node, '$$component', context, b.id('$$anchor'));
const component = build_component(
node,
'$$component',
{
...context,
state: {
...context.state,
needs_safe_props: true
}
},
b.id('$$anchor')
);
context.state.init.push(
b.stmt(
b.call(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,8 @@ export function EachBlock(node, context) {
const child_state = {
...context.state,
transform: { ...context.state.transform },
store_to_invalidate
store_to_invalidate,
needs_safe_props: true
};

/** The state used when generating the key function, if necessary */
Expand Down Expand Up @@ -308,7 +309,15 @@ export function EachBlock(node, context) {

if (node.fallback) {
args.push(
b.arrow([b.id('$$anchor')], /** @type {BlockStatement} */ (context.visit(node.fallback)))
b.arrow(
[b.id('$$anchor')],
/** @type {BlockStatement} */ (
context.visit(node.fallback, {
...context.state,
needs_safe_props: true
})
)
)
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,26 @@ export function IfBlock(node, context) {
context.state.template.push('<!>');
const statements = [];

const consequent = /** @type {BlockStatement} */ (context.visit(node.consequent));
const consequent = /** @type {BlockStatement} */ (
context.visit(node.consequent, {
...context.state,
needs_safe_props: true
})
);

const consequent_id = context.state.scope.generate('consequent');

statements.push(b.var(b.id(consequent_id), b.arrow([b.id('$$anchor')], consequent)));

let alternate_id;

if (node.alternate) {
const alternate = /** @type {BlockStatement} */ (context.visit(node.alternate));
const alternate = /** @type {BlockStatement} */ (
context.visit(node.alternate, {
...context.state,
needs_safe_props: true
})
);
alternate_id = context.state.scope.generate('alternate');
statements.push(b.var(b.id(alternate_id), b.arrow([b.id('$$anchor')], alternate)));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ export function KeyBlock(node, context) {
context.state.template.push('<!>');

const key = /** @type {Expression} */ (context.visit(node.expression));
const body = /** @type {Expression} */ (context.visit(node.fragment));
const body = /** @type {Expression} */ (
context.visit(node.fragment, { ...context.state, needs_safe_props: true })
);

context.state.init.push(
b.stmt(b.call('$.key', context.state.node, b.thunk(key), b.arrow([b.id('$$anchor')], body)))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export function SnippetBlock(node, context) {
const declarations = [];

const transform = { ...context.state.transform };
const child_state = { ...context.state, transform };
const child_state = { ...context.state, transform, needs_safe_props: true };

for (let i = 0; i < node.parameters.length; i++) {
const argument = node.parameters[i];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -370,14 +370,17 @@ export function build_component(node, component_name, context, anchor = context.
push_prop(b.init('$$legacy', b.true));
}

const props_expression =
props_and_spreads.length === 0 ||
(props_and_spreads.length === 1 && Array.isArray(props_and_spreads[0]))
? b.object(/** @type {Property[]} */ (props_and_spreads[0]) || [])
: b.call(
'$.spread_props',
...props_and_spreads.map((p) => (Array.isArray(p) ? b.object(p) : p))
);
const props = props_and_spreads.map((p) =>
Array.isArray(p)
? context.state.needs_safe_props
? b.call('$.safe_props', b.object(p))
: b.object(p)
: p
);

const props_expression = props_and_spreads.some((p) => !Array.isArray(p))
? b.call('$.spread_props', ...props)
: props[0] ?? b.object([]);

/** @param {Expression} node_id */
let fn = (node_id) => {
Expand Down
3 changes: 2 additions & 1 deletion packages/svelte/src/internal/client/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,8 @@ export {
legacy_rest_props,
spread_props,
update_pre_prop,
update_prop
update_prop,
safe_props
} from './reactivity/props.js';
export {
invalidate_store,
Expand Down
71 changes: 51 additions & 20 deletions packages/svelte/src/internal/client/reactivity/props.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,30 +7,22 @@ import {
PROPS_IS_RUNES,
PROPS_IS_UPDATED
} from '../../../constants.js';
import { get_descriptor, is_function } from '../../shared/utils.js';
import { mutable_source, set, source, update } from './sources.js';
import { derived, derived_safe_equal } from './deriveds.js';
import { legacy_mode_flag } from '../../flags/index.js';
import {
active_effect,
get,
captured_signals,
set_active_effect,
untrack,
active_reaction,
set_active_reaction
} from '../runtime.js';
import { safe_equals } from './equality.js';
define_property,
get_descriptor,
get_descriptors,
is_function
} from '../../shared/utils.js';
import { LEGACY_DERIVED_PROP, LEGACY_PROPS, STATE_SYMBOL } from '../constants.js';
import * as e from '../errors.js';
import {
BRANCH_EFFECT,
LEGACY_DERIVED_PROP,
LEGACY_PROPS,
ROOT_EFFECT,
STATE_SYMBOL
} from '../constants.js';
import { proxy } from '../proxy.js';
import { captured_signals, get, is_flushing_effect, untrack } from '../runtime.js';
import { derived, derived_safe_equal } from './deriveds.js';
import { render_effect, teardown } from './effects.js';
import { safe_equals } from './equality.js';
import { inspect_effects, mutable_source, set, source, update } from './sources.js';
import { capture_store_binding } from './store.js';
import { legacy_mode_flag } from '../../flags/index.js';

/**
* @param {((value?: number) => number)} fn
Expand Down Expand Up @@ -416,3 +408,42 @@ export function prop(props, key, flags, fallback) {
return get(current_value);
};
}

/**
*
* @param {Record<string | symbol, unknown>} props
*/
export function safe_props(props) {
let unmounting = false;
teardown(() => {
unmounting = true;
});

/** @type {typeof props} */
var values = {};

for (const key in props) {
const descriptor = /** @type {PropertyDescriptor} */ (get_descriptor(props, key));

if (descriptor.get) {
let latest = props[key];

render_effect(() => {
if (!unmounting) {
latest = props[key];
}
});

define_property(values, key, {
...descriptor,
get() {
return unmounting ? latest : props[key];
}
});
} else {
values[key] = props[key];
}
}

return values;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<script>
let { checked = $bindable(), count = $bindable() } = $props();

$effect(() => ()=>{
console.log(count, checked);
});
</script>

<p>{count}</p>

<button onclick={()=> count-- }></button>
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import { ok, test } from '../../test';
import { flushSync } from 'svelte';

export default test({
async test({ assert, target, logs }) {
const [btn1, btn2, btn3] = target.querySelectorAll('button');
let ps = [...target.querySelectorAll('p')];

for (const p of ps) {
assert.equal(p.innerHTML, '0');
}

flushSync(() => {
btn1.click();
});

// prop update normally if we are not unmounting
for (const p of ps) {
assert.equal(p.innerHTML, '1');
}

flushSync(() => {
btn3.click();
});

// binding still works and update the value correctly
for (const p of ps) {
assert.equal(p.innerHTML, '0');
}

flushSync(() => {
btn1.click();
});

flushSync(() => {
btn1.click();
});

console.warn(logs);

// the five components guarded by `count < 2` unmount and log
assert.deepEqual(logs, [1, true, 1, true, 1, true, 1, true, 1, true]);

flushSync(() => {
btn2.click();
});

// the three components guarded by `show` unmount and log
assert.deepEqual(logs, [
1,
true,
1,
true,
1,
true,
1,
true,
1,
true,
2,
true,
2,
true,
2,
true
]);
}
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
<script>
import Component from "./Component.svelte";

let show = $state(true);
let count = $state(0);
let spread = $derived({ checked: show, count });

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

<button onclick={()=> count++ }></button>
<button onclick={()=> show = !show }></button>

<!-- count with bind -->
{#if count < 2}
<Component bind:count bind:checked={show} />
{/if}

<!-- spread syntax -->
{#if count < 2}
<Component {...spread} />
{/if}

<!-- normal prop -->
{#if count < 2}
<Component {count} checked={show} />
{/if}

<!-- prop only accessed in destroy -->
{#if show}
<Component {count} checked={show} />
{/if}

<!-- dynamic component -->
<Dynamic {count} checked={show} />

<!-- dynamic component spread -->
<Dynamic {...spread} />

<!-- dynamic component with prop only accessed on destroy -->
<Dynamic2 {count} checked={show} />

<!-- dynamic component with prop only accessed on destroy spread -->
<Dynamic2 {...spread} />
Loading