fix: remove readonly validation

The readonly dev time validation results in false-negative object equality checks: The original object is proxified and thus comparisons could be between the unproxified and proxified version, which will always return false. Fixes #10372

There's also the problem that an object could be passed into a component but then passed upwards into shared state. At that point, it should no longer be readonly, but it's not possible to statically analyze these points. Fixes #10372

Lastly, the each block logic mutates an internal array and that also throws errors with the readonly logic. Fixes #10037
remove-readonly-check
Simon Holthausen 2 years ago
parent d08e05bf7f
commit f0d3740e30

@ -0,0 +1,5 @@
---
"svelte": patch
---
fix: remove readonly validation as it results in false-negative object equality checks

@ -835,8 +835,6 @@ function serialize_inline_component(node, component_name, context) {
arg = b.call('$.get', id);
}
if (context.state.options.dev) arg = b.call('$.readonly', arg);
push_prop(b.get(attribute.name, [b.return(arg)]));
} else {
push_prop(b.init(attribute.name, value));
@ -2336,17 +2334,16 @@ export const template_visitors = {
/** @type {import('estree').BlockStatement} */ (context.visit(node.fallback))
)
: b.literal(null);
const key_function =
node.key && ((each_type & EACH_ITEM_REACTIVE) !== 0 || context.state.options.dev)
? b.arrow(
[node.context.type === 'Identifier' ? node.context : b.id('$$item'), index],
b.block(
declarations.concat(
b.return(/** @type {import('estree').Expression} */ (context.visit(node.key)))
)
const key_function = node.key
? b.arrow(
[node.context.type === 'Identifier' ? node.context : b.id('$$item'), index],
b.block(
declarations.concat(
b.return(/** @type {import('estree').Expression} */ (context.visit(node.key)))
)
)
: b.literal(null);
)
: b.literal(null);
if (node.index && each_node_meta.contains_group_binding) {
// We needed to create a unique identifier for the index above, but we want to use the

@ -18,12 +18,10 @@ import {
get_prototype_of,
is_array,
is_frozen,
object_keys,
object_prototype
} from './utils.js';
export const STATE_SYMBOL = Symbol('$state');
export const READONLY_SYMBOL = Symbol('readonly');
/**
* @template T
@ -93,7 +91,7 @@ function unwrap(value, already_unwrapped = new Map()) {
const descriptors = get_descriptors(value);
already_unwrapped.set(value, obj);
for (const key of keys) {
if (key === STATE_SYMBOL || (DEV && key === READONLY_SYMBOL)) continue;
if (key === STATE_SYMBOL) continue;
if (descriptors[key].get) {
define_property(obj, key, descriptors[key]);
} else {
@ -175,9 +173,6 @@ const state_proxy_handler = {
},
get(target, prop, receiver) {
if (DEV && prop === READONLY_SYMBOL) {
return Reflect.get(target, READONLY_SYMBOL);
}
if (prop === STATE_SYMBOL) {
return Reflect.get(target, STATE_SYMBOL);
}
@ -224,9 +219,6 @@ const state_proxy_handler = {
},
has(target, prop) {
if (DEV && prop === READONLY_SYMBOL) {
return Reflect.has(target, READONLY_SYMBOL);
}
if (prop === STATE_SYMBOL) {
return true;
}
@ -250,10 +242,6 @@ const state_proxy_handler = {
},
set(target, prop, value) {
if (DEV && prop === READONLY_SYMBOL) {
target[READONLY_SYMBOL] = value;
return true;
}
const metadata = target[STATE_SYMBOL];
const s = metadata.s.get(prop);
if (s !== undefined) set(s, proxy(value, metadata.i));
@ -304,69 +292,3 @@ if (DEV) {
throw new Error('Cannot set prototype of $state object');
};
}
/**
* Expects a value that was wrapped with `proxy` and makes it readonly.
*
* @template {Record<string | symbol, any>} T
* @template {import('./types.js').ProxyReadonlyObject<T> | T} U
* @param {U} value
* @returns {Proxy<U> | U}
*/
export function readonly(value) {
const proxy = value && value[READONLY_SYMBOL];
if (proxy) {
const metadata = value[STATE_SYMBOL];
// Check that the incoming value is the same proxy that this readonly symbol was created for:
// If someone copies over the readonly symbol to a new object (using Reflect.ownKeys) the referenced
// proxy could be stale and we should not return it.
if (metadata.p === value) return proxy;
}
if (
typeof value === 'object' &&
value != null &&
!is_frozen(value) &&
STATE_SYMBOL in value && // TODO handle Map and Set as well
!(READONLY_SYMBOL in value)
) {
const proxy = new Proxy(
value,
/** @type {ProxyHandler<import('./types.js').ProxyReadonlyObject<U>>} */ (
readonly_proxy_handler
)
);
define_property(value, READONLY_SYMBOL, { value: proxy, writable: false });
return proxy;
}
return value;
}
/**
* @param {any} _
* @param {string} prop
* @returns {never}
*/
const readonly_error = (_, prop) => {
throw new Error(
`Non-bound props cannot be mutated — to make the \`${prop}\` settable, ensure the object it is used within is bound as a prop \`bind:<prop>={...}\`. Fallback values can never be mutated.`
);
};
/** @type {ProxyHandler<import('./types.js').ProxyReadonlyObject>} */
const readonly_proxy_handler = {
defineProperty: readonly_error,
deleteProperty: readonly_error,
set: readonly_error,
get(target, prop, receiver) {
const value = Reflect.get(target, prop, receiver);
if (!(prop in target)) {
return readonly(value);
}
return value;
}
};

@ -17,7 +17,7 @@ import {
PROPS_IS_RUNES,
PROPS_IS_UPDATED
} from '../../constants.js';
import { READONLY_SYMBOL, STATE_SYMBOL, proxy, readonly, unstate } from './proxy.js';
import { STATE_SYMBOL, unstate } from './proxy.js';
import { EACH_BLOCK, IF_BLOCK } from './block.js';
export const SOURCE = 1;
@ -1619,10 +1619,6 @@ export function prop(props, key, flags, initial) {
// @ts-expect-error would need a cumbersome method overload to type this
if ((flags & PROPS_IS_LAZY_INITIAL) !== 0) initial = initial();
if (DEV && runes) {
initial = readonly(proxy(/** @type {any} */ (initial)));
}
prop_value = /** @type {V} */ (initial);
if (setter) setter(prop_value);
@ -2126,10 +2122,6 @@ export function freeze(value) {
if (STATE_SYMBOL in value) {
return object_freeze(unstate(value));
}
// If the value is already read-only then just use that
if (DEV && READONLY_SYMBOL in value) {
return value;
}
// Otherwise freeze the object
object_freeze(value);
}

@ -10,7 +10,7 @@ import {
DYNAMIC_ELEMENT_BLOCK,
SNIPPET_BLOCK
} from './block.js';
import type { READONLY_SYMBOL, STATE_SYMBOL } from './proxy.js';
import type { STATE_SYMBOL } from './proxy.js';
import { DERIVED, EFFECT, RENDER_EFFECT, SOURCE, PRE_EFFECT, LAZY_PROPERTY } from './runtime.js';
// Put all internal types in this file. Once we convert to JSDoc, we can make this a d.ts file
@ -409,7 +409,7 @@ export interface ProxyMetadata<T = Record<string | symbol, any>> {
/** Immutable: Whether to use a source or mutable source under the hood */
i: boolean;
/** The associated proxy */
p: ProxyStateObject<T> | ProxyReadonlyObject<T>;
p: ProxyStateObject<T>;
/** The original target this proxy was created for */
t: T;
}
@ -417,7 +417,3 @@ export interface ProxyMetadata<T = Record<string | symbol, any>> {
export type ProxyStateObject<T = Record<string | symbol, any>> = T & {
[STATE_SYMBOL]: ProxyMetadata;
};
export type ProxyReadonlyObject<T = Record<string | symbol, any>> = ProxyStateObject<T> & {
[READONLY_SYMBOL]: ProxyMetadata;
};

@ -44,7 +44,7 @@ export * from './client/each.js';
export * from './client/render.js';
export * from './client/validate.js';
export { raf } from './client/timing.js';
export { proxy, readonly, unstate } from './client/proxy.js';
export { proxy, unstate } from './client/proxy.js';
export { create_custom_element } from './client/custom-element.js';
export {
child,

@ -0,0 +1,15 @@
import { test } from '../../test';
export default test({
html: `<button>add</button> <p>1</p><p>1</p><p>1</p>`,
async test({ assert, target }) {
const btn = target.querySelector('button');
await btn?.click();
assert.htmlEqual(
target.innerHTML,
`<button>add</button> <p>1</p><p>2</p><p>1</p><p>2</p><p>1</p><p>2</p>`
);
}
});

@ -0,0 +1,15 @@
<script>
let { array } = $props();
</script>
{#each array as number}
<p>{number.v}</p>
{/each}
{#each array as number (number)}
<p>{number.v}</p>
{/each}
{#each array as number (number.v)}
<p>{number.v}</p>
{/each}

@ -0,0 +1,12 @@
<script>
import Child from './child.svelte';
let array = $state([{v: 1}]);
const addNew = () => {
array.push({v: 2})
}
</script>
<button onclick={addNew}>add</button>
<Child {array} />

@ -0,0 +1,45 @@
import { test } from '../../test';
export default test({
html: `
<button>a true</button><button>b true</button>
<button>a true</button><button>b true</button>
<button>a true</button><button>b true</button>
`,
async test({ assert, target }) {
let [btn1, _btn2, btn3, _btn4, btn5] = target.querySelectorAll('button');
await btn1.click();
assert.htmlEqual(
target.innerHTML,
`
<button>a+ true</button><button>b true</button>
<button>a+ true</button><button>b true</button>
<button>a+ true</button><button>b true</button>
`
);
[btn1, _btn2, btn3, _btn4, btn5] = target.querySelectorAll('button');
await btn3.click();
assert.htmlEqual(
target.innerHTML,
`
<button>a++ true</button><button>b true</button>
<button>a++ true</button><button>b true</button>
<button>a++ true</button><button>b true</button>
`
);
[btn1, _btn2, btn3, _btn4, btn5] = target.querySelectorAll('button');
await btn5.click();
assert.htmlEqual(
target.innerHTML,
`
<button>a+++ true</button><button>b true</button>
<button>a+++ true</button><button>b true</button>
<button>a+++ true</button><button>b true</button>
`
);
}
});

@ -0,0 +1,7 @@
<script>
let {item, items, onclick} = $props()
</script>
<button {onclick}>
{item.name} {items.includes(item)}
</button>

@ -0,0 +1,19 @@
<script>
import Item from './item.svelte'
let items = $state([{name: 'a'}, {name: 'b'}]);
</script>
<!-- test that each block doesn't mess with item identity -->
{#each items as item (item)}
<Item {item} {items} onclick={() => item.name = item.name + '+'} />
{/each}
{#each items as item (item.name)}
<Item {item} {items} onclick={() => {console.log('hello'); item.name = item.name + '+'}} />
{/each}
{#each items as item}
<Item {item} {items} onclick={() => item.name = item.name + '+'} />
{/each}

@ -1,8 +0,0 @@
<script>
/** @type {{ object: { count: number }}} */
let { object } = $props();
</script>
<button onclick={() => object = { count: object.count + 1 } }>
clicks: {object.count}
</button>

@ -1,22 +0,0 @@
import { test } from '../../test';
// Tests that readonly bails on setters/classes
export default test({
html: `<button>clicks: 0</button><button>clicks: 0</button>`,
compileOptions: {
dev: true
},
async test({ assert, target }) {
const [btn1, btn2] = target.querySelectorAll('button');
await btn1.click();
await btn2.click();
assert.htmlEqual(target.innerHTML, `<button>clicks: 1</button><button>clicks: 1</button>`);
await btn1.click();
await btn2.click();
assert.htmlEqual(target.innerHTML, `<button>clicks: 2</button><button>clicks: 2</button>`);
}
});

@ -1,25 +0,0 @@
<script>
import Counter from './Counter.svelte';
function createCounter() {
let count = $state(0)
return {
get count() {
return count;
},
set count(upd) {
count = upd
}
}
}
class CounterClass {
count = $state(0);
}
const counterSetter = createCounter();
const counterClass = new CounterClass();
</script>
<Counter object={counterSetter} />
<Counter object={counterClass} />

@ -1,8 +0,0 @@
<script>
/** @type {{ object?: { count: number }}} */
let { object = { count: 0 } } = $props();
</script>
<button onclick={() => object = { count: object.count + 1 } }>
clicks: {object.count}
</button>

@ -1,19 +0,0 @@
import { test } from '../../test';
export default test({
html: `<button>clicks: 0</button>`,
compileOptions: {
dev: true
},
async test({ assert, target }) {
const btn = target.querySelector('button');
await btn?.click();
assert.htmlEqual(target.innerHTML, `<button>clicks: 1</button>`);
await btn?.click();
assert.htmlEqual(target.innerHTML, `<button>clicks: 2</button>`);
}
});

@ -1,5 +0,0 @@
<script>
import Counter from './Counter.svelte';
</script>
<Counter />

@ -1,8 +0,0 @@
<script>
/** @type {{ object?: { count: number }}} */
let { object = { count: 0 } } = $props();
</script>
<button onclick={() => object.count += 1}>
clicks: {object.count}
</button>

@ -1,19 +0,0 @@
import { test } from '../../test';
export default test({
html: `<button>clicks: 0</button>`,
compileOptions: {
dev: true
},
async test({ assert, target }) {
const btn = target.querySelector('button');
await btn?.click();
assert.htmlEqual(target.innerHTML, `<button>clicks: 0</button>`);
},
runtime_error:
'Non-bound props cannot be mutated — to make the `count` settable, ensure the object it is used within is bound as a prop `bind:<prop>={...}`. Fallback values can never be mutated.'
});

@ -1,5 +0,0 @@
<script>
import Counter from './Counter.svelte';
</script>
<Counter />

@ -1,8 +0,0 @@
<script>
/** @type {{ object: { count: number }}} */
let { object } = $props();
</script>
<button onclick={() => object.count += 1}>
clicks: {object.count}
</button>

@ -1,19 +0,0 @@
import { test } from '../../test';
export default test({
html: `<button>clicks: 0</button>`,
compileOptions: {
dev: true
},
async test({ assert, target }) {
const btn = target.querySelector('button');
await btn?.click();
assert.htmlEqual(target.innerHTML, `<button>clicks: 0</button>`);
},
runtime_error:
'Non-bound props cannot be mutated — to make the `count` settable, ensure the object it is used within is bound as a prop `bind:<prop>={...}`. Fallback values can never be mutated.'
});

@ -1,7 +0,0 @@
<script>
import Counter from './Counter.svelte';
const object = $state({ count: 0 });
</script>
<Counter {object} />
Loading…
Cancel
Save