mirror of https://github.com/sveltejs/svelte
fix: stack-trace-based readonly validation (#10464)
* 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 * reinstate tests * track ownership of state and mutations * working? * remove old changeset * tidy * error * simplify * fix * fix * fix * tidy * make it a warning * rename test * remove unused test * update tests * slap ts-expect-error everywhere, because its too finicky otherwise * oops * oh no the hall monitor is here * only call add_owner in dev * only owners can transfer ownership * simplify * fixes * tidy up * fix type error * while we're at it * rename file * rename functions * add some comments * move ownership checking logic * ugh eslint * more detailed message * only add filename in dev * comment * update tests * move more code * undo change to sourcemap tests * allow proxy to have multiple owners * use SignalDebug * i was doing this all wrong * tidy up * implement inheritance * fix * tidy up * update filename stuff * changeset --------- Co-authored-by: Simon Holthausen <simon.holthausen@vercel.com> Co-authored-by: Rich Harris <rich.harris@vercel.com>pull/10568/head
Rich Harris
2 years ago
committed by
GitHub
parent
f1550b2ea3
commit
8613a6f28a
@ -0,0 +1,5 @@
|
||||
---
|
||||
'svelte': patch
|
||||
---
|
||||
|
||||
fix: replace proxy-based readonly validation with stack-trace-based ownership tracking
|
||||
@ -0,0 +1,154 @@
|
||||
/** @typedef {{ file: string, line: number, column: number }} Location */
|
||||
|
||||
import { STATE_SYMBOL } from '../proxy.js';
|
||||
import { untrack } from '../runtime.js';
|
||||
|
||||
/** @type {Record<string, Array<{ start: Location, end: Location, component: Function }>>} */
|
||||
const boundaries = {};
|
||||
|
||||
const chrome_pattern = /at (?:.+ \()?(.+):(\d+):(\d+)\)?$/;
|
||||
const firefox_pattern = /@(.+):(\d+):(\d+)$/;
|
||||
|
||||
export function get_stack() {
|
||||
const stack = new Error().stack;
|
||||
if (!stack) return null;
|
||||
|
||||
const entries = [];
|
||||
|
||||
for (const line of stack.split('\n')) {
|
||||
let match = chrome_pattern.exec(line) ?? firefox_pattern.exec(line);
|
||||
|
||||
if (match) {
|
||||
entries.push({
|
||||
file: match[1],
|
||||
line: +match[2],
|
||||
column: +match[3]
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
return entries;
|
||||
}
|
||||
|
||||
/**
|
||||
* Determines which `.svelte` component is responsible for a given state change
|
||||
* @returns {Function | null}
|
||||
*/
|
||||
export function get_component() {
|
||||
const stack = get_stack();
|
||||
if (!stack) return null;
|
||||
|
||||
for (const entry of stack) {
|
||||
const modules = boundaries[entry.file];
|
||||
if (!modules) continue;
|
||||
|
||||
for (const module of modules) {
|
||||
if (module.start.line < entry.line && module.end.line > entry.line) {
|
||||
return module.component;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Together with `mark_module_end`, this function establishes the boundaries of a `.svelte` file,
|
||||
* such that subsequent calls to `get_component` can tell us which component is responsible
|
||||
* for a given state change
|
||||
* @param {Function} component
|
||||
*/
|
||||
export function mark_module_start(component) {
|
||||
const start = get_stack()?.[2];
|
||||
|
||||
if (start) {
|
||||
(boundaries[start.file] ??= []).push({
|
||||
start,
|
||||
// @ts-expect-error
|
||||
end: null,
|
||||
component
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
export function mark_module_end() {
|
||||
const end = get_stack()?.[2];
|
||||
|
||||
if (end) {
|
||||
// @ts-expect-error
|
||||
boundaries[end.file].at(-1).end = end;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
*
|
||||
* @param {any} object
|
||||
* @param {any} owner
|
||||
*/
|
||||
export function add_owner(object, owner) {
|
||||
untrack(() => {
|
||||
add_owner_to_object(object, owner);
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* @param {any} object
|
||||
* @param {Function} owner
|
||||
*/
|
||||
function add_owner_to_object(object, owner) {
|
||||
if (object?.[STATE_SYMBOL]?.o && !object[STATE_SYMBOL].o.has(owner)) {
|
||||
object[STATE_SYMBOL].o.add(owner);
|
||||
|
||||
for (const key in object) {
|
||||
add_owner_to_object(object[key], owner);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* @param {any} object
|
||||
*/
|
||||
export function strip_owner(object) {
|
||||
untrack(() => {
|
||||
strip_owner_from_object(object);
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* @param {any} object
|
||||
*/
|
||||
function strip_owner_from_object(object) {
|
||||
if (object?.[STATE_SYMBOL]?.o) {
|
||||
object[STATE_SYMBOL].o = null;
|
||||
|
||||
for (const key in object) {
|
||||
strip_owner(object[key]);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* @param {Set<Function>} owners
|
||||
*/
|
||||
export function check_ownership(owners) {
|
||||
const component = get_component();
|
||||
|
||||
if (component && !owners.has(component)) {
|
||||
let original = [...owners][0];
|
||||
|
||||
let message =
|
||||
// @ts-expect-error
|
||||
original.filename !== component.filename
|
||||
? // @ts-expect-error
|
||||
`${component.filename} mutated a value owned by ${original.filename}. This is strongly discouraged`
|
||||
: 'Mutating a value outside the component that created it is strongly discouraged';
|
||||
|
||||
// eslint-disable-next-line no-console
|
||||
console.warn(
|
||||
`${message}. Consider passing values to child components with \`bind:\`, or use a callback instead.`
|
||||
);
|
||||
|
||||
// eslint-disable-next-line no-console
|
||||
console.trace();
|
||||
}
|
||||
}
|
||||
@ -0,0 +1,47 @@
|
||||
import { test } from '../../test';
|
||||
|
||||
/** @type {typeof console.warn} */
|
||||
let warn;
|
||||
|
||||
/** @type {typeof console.trace} */
|
||||
let trace;
|
||||
|
||||
/** @type {any[]} */
|
||||
let warnings = [];
|
||||
|
||||
export default test({
|
||||
html: `<button>clicks: 0</button>`,
|
||||
|
||||
compileOptions: {
|
||||
dev: true
|
||||
},
|
||||
|
||||
before_test: () => {
|
||||
warn = console.warn;
|
||||
trace = console.trace;
|
||||
|
||||
console.warn = (...args) => {
|
||||
warnings.push(...args);
|
||||
};
|
||||
|
||||
console.trace = () => {};
|
||||
},
|
||||
|
||||
after_test: () => {
|
||||
console.warn = warn;
|
||||
console.trace = trace;
|
||||
|
||||
warnings = [];
|
||||
},
|
||||
|
||||
async test({ assert, target }) {
|
||||
const btn = target.querySelector('button');
|
||||
await btn?.click();
|
||||
|
||||
assert.htmlEqual(target.innerHTML, `<button>clicks: 1</button>`);
|
||||
|
||||
assert.deepEqual(warnings, [
|
||||
'.../samples/non-local-mutation-discouraged/Counter.svelte mutated a value owned by .../samples/non-local-mutation-discouraged/main.svelte. This is strongly discouraged. Consider passing values to child components with `bind:`, or use a callback instead.'
|
||||
]);
|
||||
}
|
||||
});
|
||||
@ -0,0 +1,7 @@
|
||||
<script>
|
||||
import { global } from './state.svelte.js';
|
||||
</script>
|
||||
|
||||
<button onclick={() => global.object.count += 1}>
|
||||
clicks: {global.object.count}
|
||||
</button>
|
||||
@ -0,0 +1,9 @@
|
||||
<script>
|
||||
import Counter from './Counter.svelte';
|
||||
import { global } from './state.svelte.js';
|
||||
|
||||
let object = $state({ count: 0 });
|
||||
global.object = object;
|
||||
</script>
|
||||
|
||||
<Counter />
|
||||
@ -0,0 +1,3 @@
|
||||
export let global = $state({
|
||||
object: { count: -1 }
|
||||
});
|
||||
@ -0,0 +1,37 @@
|
||||
import { test } from '../../test';
|
||||
|
||||
/** @type {typeof console.warn} */
|
||||
let warn;
|
||||
|
||||
/** @type {any[]} */
|
||||
let warnings = [];
|
||||
|
||||
export default test({
|
||||
html: `<button>clicks: 0</button>`,
|
||||
|
||||
compileOptions: {
|
||||
dev: true
|
||||
},
|
||||
|
||||
before_test: () => {
|
||||
warn = console.warn;
|
||||
|
||||
console.warn = (...args) => {
|
||||
warnings.push(...args);
|
||||
};
|
||||
},
|
||||
|
||||
after_test: () => {
|
||||
console.warn = warn;
|
||||
warnings = [];
|
||||
},
|
||||
|
||||
async test({ assert, target }) {
|
||||
const btn = target.querySelector('button');
|
||||
await btn?.click();
|
||||
|
||||
assert.htmlEqual(target.innerHTML, `<button>clicks: 1</button>`);
|
||||
|
||||
assert.deepEqual(warnings, []);
|
||||
}
|
||||
});
|
||||
@ -0,0 +1,7 @@
|
||||
<script>
|
||||
import Counter from './Counter.svelte';
|
||||
|
||||
let object = $state({ count: 0 });
|
||||
</script>
|
||||
|
||||
<Counter bind:object={object} />
|
||||
@ -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,30 @@
|
||||
import { test } from '../../test';
|
||||
|
||||
export default test({
|
||||
html: `
|
||||
<button>mutate: 0</button>
|
||||
<button>reassign: 0</button>
|
||||
`,
|
||||
|
||||
async test({ assert, target }) {
|
||||
const [btn1, btn2] = target.querySelectorAll('button');
|
||||
|
||||
await btn1?.click();
|
||||
assert.htmlEqual(
|
||||
target.innerHTML,
|
||||
`
|
||||
<button>mutate: 0</button>
|
||||
<button>reassign: 0</button>
|
||||
`
|
||||
);
|
||||
|
||||
await btn2?.click();
|
||||
assert.htmlEqual(
|
||||
target.innerHTML,
|
||||
`
|
||||
<button>mutate: 2</button>
|
||||
<button>reassign: 2</button>
|
||||
`
|
||||
);
|
||||
}
|
||||
});
|
||||
@ -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,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 += 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,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.'
|
||||
});
|
||||
Loading…
Reference in new issue