msb-public
/
svelte
mirror of https://github.com/sveltejs/svelte
1
0
Code Issues Projects Releases Wiki Activity

fix: add test and change strategy

Browse Source
fix-props-wrong-value-onunmount
paoloricciuti 2 years ago
parent a73a619714
commit 48adf81220
9 changed files with 194 additions and 79 deletions
Show Stats Download Patch File Download Diff File

3
packages/svelte/src/compiler/phases/3-transform/client/types.d.ts vendored
Unescape View File

@ -23,8 +23,6 @@ export interface ClientTransformState extends TransformState {
* us to rewrite `this.foo` as `this.#foo.value`
*/
readonly in_constructor: boolean;
readonly safe_props_ids?: Map<string, Expression>;
readonly safe_props_name?: string;
readonly transform: Record<
string,
@ -47,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) */

21
packages/svelte/src/compiler/phases/3-transform/client/visitors/Component.js
Unescape View File

@ -10,10 +10,6 @@ import { build_component } from './shared/component.js';
*/
export function Component(node, context) {
if (node.metadata.dynamic) {
let safe_props_ids = new Map();
const safe_props_name = context.state.scope.generate('$$safe_props');
// Handle dynamic references to what seems like static inline components
const component = build_component(
node,
@ -22,8 +18,7 @@ export function Component(node, context) {
...context,
state: {
...context.state,
safe_props_ids,
safe_props_name
needs_safe_props: true
}
},
b.id('$$anchor')
@ -36,19 +31,7 @@ export function Component(node, context) {
// TODO use untrack here to not update when binding changes?
// Would align with Svelte 4 behavior, but it's arguably nicer/expected to update this
b.thunk(/** @type {Expression} */ (context.visit(b.member_id(node.name)))),
b.arrow(
[b.id('$$anchor'), b.id('$$component')],
b.block([
b.const(
safe_props_name,
b.call(
'$.safe_props',
b.object([...safe_props_ids].map(([name, id]) => b.get(name, [b.return(id)])))
)
),
component
])
)
b.arrow([b.id('$$anchor'), b.id('$$component')], b.block([component]))
)
)
);

36
packages/svelte/src/compiler/phases/3-transform/client/visitors/Identifier.js
Unescape View File

@ -9,9 +9,9 @@ import { build_getter } from '../utils.js';
* @param {Context} context
*/
export function Identifier(node, context) {
let parent = context.path.at(-1);
const parent = /** @type {Node} */ (context.path.at(-1));
if (is_reference(node, /** @type {Node} */ (parent))) {
if (is_reference(node, parent)) {
if (node.name === '$$props') {
return b.id('$$sanitized_props');
}
@ -36,36 +36,6 @@ export function Identifier(node, context) {
}
}
const getter = build_getter(node, context.state);
if (
// this means we are inside an if or as an attribute of a dynamic component
// and we want to access `$$safe_props` to allow for the component to access them
// after destructuring
context.state.safe_props_name != null &&
context.state.safe_props_ids != null &&
// the parent can either be a component/svelte component in that case we
// check if this identifier is one of the attributes
(((parent?.type === 'Component' || parent?.type === 'SvelteComponent') &&
parent.attributes.some(
(el) =>
(el.type === 'Attribute' &&
typeof el.value !== 'boolean' &&
!Array.isArray(el.value) &&
el.value.expression === node) ||
(el.type === 'BindDirective' && el.expression === node)
)) ||
// or a spread and we check the expression
(parent?.type === 'SpreadAttribute' && parent.expression === node)) &&
// we also don't want to transform bindings that are defined withing the if block
// itself (for example an each local variable)
!binding?.references[0].path.some((node) => node.type === 'IfBlock')
) {
// we store the getter in the safe props id and return an access to `$$safe_props.name`
context.state.safe_props_ids.set(node.name, getter);
return b.member(b.id(context.state.safe_props_name), b.id(node.name));
}
return getter;
return build_getter(node, context.state);
}
}

25
packages/svelte/src/compiler/phases/3-transform/client/visitors/IfBlock.js
Unescape View File

@ -11,29 +11,13 @@ export function IfBlock(node, context) {
context.state.template.push('<!>');
const statements = [];
let safe_props_ids = new Map();
const safe_props_id = context.state.scope.generate('$$safe_props');
const consequent = /** @type {BlockStatement} */ (
context.visit(node.consequent, {
...context.state,
safe_props_ids,
safe_props_name: safe_props_id
needs_safe_props: true
})
);
if (consequent.body.length > 0 && safe_props_ids) {
consequent.body.unshift(
b.const(
safe_props_id,
b.call(
'$.safe_props',
b.object([...safe_props_ids].map(([name, id]) => b.get(name, [b.return(id)])))
)
)
);
}
const consequent_id = context.state.scope.generate('consequent');
statements.push(b.var(b.id(consequent_id), b.arrow([b.id('$$anchor')], consequent)));
@ -41,7 +25,12 @@ export function IfBlock(node, context) {
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)));
}

62
packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js
Unescape View File

@ -92,6 +92,21 @@ export function build_component(node, component_name, context, anchor = context.
}
}
let safe_props_ids = new Map();
let safe_props_name = context.state.scope.generate('$$safe_props');
/**
* @param {string} name
* @param {Expression} expression
*/
function safe_propify(name, expression) {
if (context.state.needs_safe_props) {
safe_props_ids.set(name, expression);
return b.member(b.id(safe_props_name), b.id(name));
}
return expression;
}
for (const attribute of node.attributes) {
if (attribute.type === 'LetDirective') {
if (!slot_scope_applies_to_itself) {
@ -118,13 +133,14 @@ export function build_component(node, component_name, context, anchor = context.
if (attribute.metadata.expression.has_state) {
let value = expression;
const name = context.state.scope.generate('spread_element');
if (attribute.metadata.expression.has_call) {
const id = b.id(context.state.scope.generate('spread_element'));
const id = b.id(name);
context.state.init.push(b.var(id, b.call('$.derived', b.thunk(value))));
value = b.call('$.get', id);
}
props_and_spreads.push(b.thunk(value));
props_and_spreads.push(b.thunk(safe_propify(name, value)));
} else {
props_and_spreads.push(expression);
}
@ -172,7 +188,7 @@ export function build_component(node, component_name, context, anchor = context.
);
if (has_state) {
push_prop(b.get(attribute.name, [b.return(value)]));
push_prop(b.get(attribute.name, [b.return(safe_propify(attribute.name, value))]));
} else {
push_prop(b.init(attribute.name, value));
}
@ -205,7 +221,9 @@ export function build_component(node, component_name, context, anchor = context.
context.state.init.push(b.var(get_id, get));
context.state.init.push(b.var(set_id, set));
push_prop(b.get(attribute.name, [b.return(b.call(get_id))]));
push_prop(
b.get(attribute.name, [b.return(safe_propify(attribute.name, b.call(get_id)))])
);
push_prop(b.set(attribute.name, [b.stmt(b.call(set_id, b.id('$$value')))]));
}
} else {
@ -228,11 +246,17 @@ export function build_component(node, component_name, context, anchor = context.
// Delay prop pushes so bindings come at the end, to avoid spreads overwriting them
if (is_store_sub) {
push_prop(
b.get(attribute.name, [b.stmt(b.call('$.mark_store_binding')), b.return(expression)]),
b.get(attribute.name, [
b.stmt(b.call('$.mark_store_binding')),
b.return(safe_propify(attribute.name, expression))
]),
true
);
} else {
push_prop(b.get(attribute.name, [b.return(expression)]), true);
push_prop(
b.get(attribute.name, [b.return(safe_propify(attribute.name, expression))]),
true
);
}
const assignment = b.assignment(
@ -403,6 +427,32 @@ export function build_component(node, component_name, context, anchor = context.
const statements = [...snippet_declarations];
if (safe_props_ids.size > 0) {
// if it is a dynamic component we need to include the safe props call inside the component
// function otherwise in the init (which in case of the if will be in the consequent/alternate function)
if (component_name === '$$component') {
statements.push(
b.const(
safe_props_name,
b.call(
'$.safe_props',
b.object([...safe_props_ids].map(([name, id]) => b.get(name, [b.return(id)])))
)
)
);
} else {
context.state.init.push(
b.const(
safe_props_name,
b.call(
'$.safe_props',
b.object([...safe_props_ids].map(([name, id]) => b.get(name, [b.return(id)])))
)
)
);
}
}
if (node.type === 'SvelteComponent') {
const prev = fn;

2
packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/element.js
Unescape View File

@ -185,7 +185,7 @@ export function build_attribute_value(value, context, memoize = (value) => value
return { value: b.literal(chunk.data), has_state: false };
}
let expression = /** @type {Expression} */ (context.visit(chunk.expression, context.state));
let expression = /** @type {Expression} */ (context.visit(chunk.expression));
return {
value: memoize(expression, chunk.metadata.expression),

11
packages/svelte/tests/runtime-runes/samples/ondestroy-prop-access/Component.svelte
Unescape View File

@ -0,0 +1,11 @@
<script>
let { checked = $bindable(), count = $bindable() } = $props();
$effect(() => ()=>{
console.log(count, checked);
});
</script>
<p>{count}</p>
<button onclick={()=> count-- }></button>

68
packages/svelte/tests/runtime-runes/samples/ondestroy-prop-access/_config.js
Unescape View File

@ -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
]);
}
});

45
packages/svelte/tests/runtime-runes/samples/ondestroy-prop-access/main.svelte
Unescape View File

@ -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} />
Write Preview
Loading…
Cancel
Save