Skip to content

fix: ensure #if blocks correctly guard against nullable prop values #16140

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

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,7 @@ export function SlotElement(node, context) {
// Let bindings first, they can be used on attributes
context.state.init.push(...lets);

const props_expression =
spreads.length === 0 ? b.object(props) : b.call('$.spread_props', b.object(props), ...spreads);
const props_expression = b.call('$.props', b.object(props), ...spreads);

const fallback =
node.fragment.nodes.length === 0
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
import { dev, is_ignored } from '../../../../../state.js';
import { get_attribute_chunks, object } from '../../../../../utils/ast.js';
import * as b from '#compiler/builders';
import { build_bind_this, memoize_expression, validate_binding } from '../shared/utils.js';
import { build_attribute_value } from '../shared/element.js';
import { build_bind_this, memoize_expression, validate_binding } from './utils.js';
import { build_attribute_value } from './element.js';
import { build_event_handler } from './events.js';
import { determine_slot } from '../../../../../utils/slot.js';

Expand Down Expand Up @@ -399,14 +399,10 @@ export function build_component(node, component_name, 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_expression = b.call(
'$.props',
...props_and_spreads.map((p) => (Array.isArray(p) ? b.object(p) : p))
);

/** @param {Expression} node_id */
let fn = (node_id) => {
Expand Down
9 changes: 2 additions & 7 deletions packages/svelte/src/internal/client/context.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,16 +101,15 @@ export function getAllContexts() {
* @returns {void}
*/
export function push(props, runes = false, fn) {
var ctx = (component_context = {
component_context = {
p: component_context,
c: null,
d: false,
e: null,
m: false,
s: props,
x: null,
l: null
});
};

if (legacy_mode_flag && !runes) {
component_context.l = {
Expand All @@ -121,10 +120,6 @@ export function push(props, runes = false, fn) {
};
}

teardown(() => {
/** @type {ComponentContext} */ (ctx).d = true;
});

if (DEV) {
// component function
component_context.function = fn;
Expand Down
2 changes: 1 addition & 1 deletion packages/svelte/src/internal/client/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ export {
prop,
rest_props,
legacy_rest_props,
spread_props,
props,
update_pre_prop,
update_prop
} from './reactivity/props.js';
Expand Down
2 changes: 1 addition & 1 deletion packages/svelte/src/internal/client/reactivity/deriveds.js
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ let stack = [];
* @param {Derived} derived
* @returns {Effect | null}
*/
function get_derived_parent_effect(derived) {
export function get_derived_parent_effect(derived) {
var parent = derived.parent;
while (parent !== null) {
if ((parent.f & DERIVED) === 0) {
Expand Down
55 changes: 40 additions & 15 deletions packages/svelte/src/internal/client/reactivity/props.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,17 @@ import {
} 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 { get, captured_signals, untrack } from '../runtime.js';
import { derived, derived_safe_equal, get_derived_parent_effect } from './deriveds.js';
import { active_effect, captured_signals, get, untrack } from '../runtime.js';
import { safe_equals } from './equality.js';
import * as e from '../errors.js';
import { LEGACY_DERIVED_PROP, LEGACY_PROPS, STATE_SYMBOL } from '#client/constants';
import {
DESTROYED,
INERT,
LEGACY_DERIVED_PROP,
LEGACY_PROPS,
STATE_SYMBOL
} from '#client/constants';
import { proxy } from '../proxy.js';
import { capture_store_binding } from './store.js';
import { legacy_mode_flag } from '../../flags/index.js';
Expand Down Expand Up @@ -159,16 +165,17 @@ export function legacy_rest_props(props, exclude) {
* The proxy handler for spread props. Handles the incoming array of props
* that looks like `() => { dynamic: props }, { static: prop }, ..` and wraps
* them so that the whole thing is passed to the component as the `$$props` argument.
* @template {Record<string | symbol, unknown>} T
* @type {ProxyHandler<{ props: Array<T | (() => T)> }>}}
* @typedef {Record<string | symbol, unknown>} AnyProps
* @type {ProxyHandler<{ props: Array<AnyProps | (() => AnyProps)>, old_props: AnyProps, destroyed: boolean }>}}
*/
const spread_props_handler = {
get(target, key) {
if (target.destroyed && key in target.old_props) return target.old_props[key];
let i = target.props.length;
while (i--) {
let p = target.props[i];
if (is_function(p)) p = p();
if (typeof p === 'object' && p !== null && key in p) return p[key];
if (typeof p === 'object' && p !== null && key in p) return (target.old_props[key] = p[key]);
}
},
set(target, key, value) {
Expand All @@ -178,7 +185,7 @@ const spread_props_handler = {
if (is_function(p)) p = p();
const desc = get_descriptor(p, key);
if (desc && desc.set) {
desc.set(value);
desc.set((target.old_props[key] = value));
return true;
}
}
Expand Down Expand Up @@ -237,16 +244,34 @@ const spread_props_handler = {
* @param {Array<Record<string, unknown> | (() => Record<string, unknown>)>} props
* @returns {any}
*/
export function spread_props(...props) {
return new Proxy({ props }, spread_props_handler);
export function props(...props) {
const effect = active_effect;
return new Proxy(
{
props,
old_props: untrack(() => {
const old_props = {};
for (let p of props) {
if (typeof p === 'function') p = p();
Object.assign(old_props, p);
}
return old_props;
}),
get destroyed() {
return effect ? (effect.f & (DESTROYED | INERT)) !== 0 : false;
}
},
spread_props_handler
);
}

/**
* @param {Derived} current_value
* @returns {boolean}
* @param {Derived} derived
*/
function has_destroyed_component_ctx(current_value) {
return current_value.ctx?.d ?? false;
function is_paused_or_destroyed(derived) {
const parent = get_derived_parent_effect(derived);
if (!parent) return false;
return (parent.f & (DESTROYED | INERT)) !== 0;
}

/**
Expand Down Expand Up @@ -414,7 +439,7 @@ export function prop(props, key, flags, fallback) {
fallback_value = new_value;
}

if (has_destroyed_component_ctx(current_value)) {
if (is_paused_or_destroyed(current_value)) {
return value;
}

Expand All @@ -424,7 +449,7 @@ export function prop(props, key, flags, fallback) {
return value;
}

if (has_destroyed_component_ctx(current_value)) {
if (is_paused_or_destroyed(current_value)) {
return current_value.v;
}

Expand Down
12 changes: 0 additions & 12 deletions packages/svelte/src/internal/client/reactivity/sources.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import {
reaction_sources,
check_dirtiness,
untracking,
is_destroying_effect,
push_reaction_value
} from '../runtime.js';
import { equals, safe_equals } from './equality.js';
Expand All @@ -38,9 +37,6 @@ import { execute_derived } from './deriveds.js';

export let inspect_effects = new Set();

/** @type {Map<Source, any>} */
export const old_values = new Map();

/**
* @param {Set<any>} v
*/
Expand Down Expand Up @@ -160,14 +156,6 @@ export function set(source, value, should_proxy = false) {
*/
export function internal_set(source, value) {
if (!source.equals(value)) {
var old_value = source.v;

if (is_destroying_effect) {
old_values.set(source, value);
} else {
old_values.set(source, old_value);
}

source.v = value;

if (DEV && tracing_mode_flag) {
Expand Down
7 changes: 1 addition & 6 deletions packages/svelte/src/internal/client/runtime.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import {
EFFECT_IS_UPDATING
} from './constants.js';
import { flush_tasks } from './dom/task.js';
import { internal_set, old_values } from './reactivity/sources.js';
import { internal_set } from './reactivity/sources.js';
import { destroy_derived_effects, update_derived } from './reactivity/deriveds.js';
import * as e from './errors.js';

Expand Down Expand Up @@ -535,7 +535,6 @@ function flush_queued_root_effects() {
var collected_effects = process_effects(root_effects[i]);
flush_queued_effects(collected_effects);
}
old_values.clear();
}
} finally {
is_flushing = false;
Expand Down Expand Up @@ -800,10 +799,6 @@ export function get(signal) {
}
}

if (is_destroying_effect && old_values.has(signal)) {
return old_values.get(signal);
}

return signal.v;
}

Expand Down
2 changes: 0 additions & 2 deletions packages/svelte/src/internal/client/types.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@ export type ComponentContext = {
p: null | ComponentContext;
/** context */
c: null | Map<unknown, unknown>;
/** destroyed */
d: boolean;
/** deferred effects */
e: null | Array<{
fn: () => void | (() => void);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ export default test({
'up',
{ foo: false, bar: false },
'down',
{ foo: false, bar: false },
// TODO the test should be deleted as there's no more concept of "teardown stale value"
{ foo: true, bar: true },
'up',
{ foo: true, bar: true }
]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,14 @@ export default test({
});

await Promise.resolve();
assert.deepEqual(logs, ['top level', 'inner', 0, 'destroy inner', 0, 'destroy outer', 0]);
assert.deepEqual(logs, [
'top level',
'inner',
0,
'destroy inner',
undefined,
'destroy outer',
undefined
]);
}
});
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,17 @@ export default function Bind_component_snippet($$anchor) {
var fragment = root();
var node = $.first_child(fragment);

TextInput(node, {
TextInput(node, $.props({
get value() {
return $.get(value);
},
set value($$value) {
$.set(value, $$value, true);
}
});
}));

var text_1 = $.sibling(node);

$.template_effect(() => $.set_text(text_1, ` value: ${$.get(value) ?? ''}`));
$.append($$anchor, fragment);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@ import 'svelte/internal/flags/legacy';
import * as $ from 'svelte/internal/client';

export default function Bind_this($$anchor) {
$.bind_this(Foo($$anchor, { $$legacy: true }), ($$value) => foo = $$value, () => foo);
}
$.bind_this(Foo($$anchor, $.props({ $$legacy: true })), ($$value) => foo = $$value, () => foo);
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ export default function Function_prop_no_getter($$anchor) {

const plusOne = (num) => num + 1;

Button($$anchor, {
Button($$anchor, $.props({
onmousedown: () => $.set(count, $.get(count) + 1),
onmouseup,
onmouseenter: () => $.set(count, plusOne($.get(count)), true),
Expand All @@ -23,5 +23,5 @@ export default function Function_prop_no_getter($$anchor) {
$.append($$anchor, text);
},
$$slots: { default: true }
});
}
}));
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,6 @@ export default function Purity($$anchor) {

var node = $.sibling(p_1, 2);

Child(node, { prop: encodeURIComponent('hello') });
Child(node, $.props({ prop: encodeURIComponent('hello') }));
$.append($$anchor, fragment);
}
}
Loading