Skip to content

fix: remove readonly validation #10378

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

Closed
wants to merge 2 commits into from
Closed
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/tough-houses-sparkle.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"svelte": patch
---

fix: remove readonly validation as it results in false-negative object equality checks
Original file line number Diff line number Diff line change
Expand Up @@ -842,8 +842,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));
Expand Down Expand Up @@ -2376,17 +2374,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
Expand Down
79 changes: 1 addition & 78 deletions packages/svelte/src/internal/client/proxy.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import {
} from './utils.js';

export const STATE_SYMBOL = Symbol('$state');
export const READONLY_SYMBOL = Symbol('readonly');

/**
* @template T
Expand Down Expand Up @@ -95,7 +94,7 @@ function unwrap(value, already_unwrapped) {
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 {
Expand Down Expand Up @@ -163,9 +162,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);
}
Expand Down Expand Up @@ -212,9 +208,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;
}
Expand All @@ -238,10 +231,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));
Expand Down Expand Up @@ -292,69 +281,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;
}
};
10 changes: 1 addition & 9 deletions packages/svelte/src/internal/client/runtime.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -1580,10 +1580,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);
Expand Down Expand Up @@ -2114,10 +2110,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);
}
Expand Down
8 changes: 2 additions & 6 deletions packages/svelte/src/internal/client/types.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 } from './runtime.js';

// Put all internal types in this file. Once we convert to JSDoc, we can make this a d.ts file
Expand Down Expand Up @@ -403,15 +403,11 @@ 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;
}

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;
};
2 changes: 1 addition & 1 deletion packages/svelte/src/internal/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
@@ -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>`
);
}
});
Original file line number Diff line number Diff line change
@@ -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}
Original file line number Diff line number Diff line change
@@ -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} />
Original file line number Diff line number Diff line change
@@ -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>
`
);
}
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<script>
let {item, items, onclick} = $props()
</script>

<button {onclick}>
{item.name} {items.includes(item)}
</button>
Original file line number Diff line number Diff line change
@@ -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}

This file was deleted.

This file was deleted.

Loading