Skip to content

fix: prevent false positive ownership warning when reassigning state #11812

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

Merged
merged 1 commit into from
May 29, 2024
Merged
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/modern-apricots-promise.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"svelte": patch
---

fix: prevent buggy ownership warning when reassigning state
27 changes: 23 additions & 4 deletions packages/svelte/src/compiler/phases/3-transform/client/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ export function serialize_set_binding(node, context, fallback, options) {
assignment.right =
private_state.kind === 'frozen_state'
? b.call('$.freeze', value)
: b.call('$.proxy', value);
: serialize_proxy_reassignment(value, private_state.id, state);
return assignment;
}
}
Expand All @@ -216,7 +216,7 @@ export function serialize_set_binding(node, context, fallback, options) {
should_proxy_or_freeze(value, context.state.scope)
? private_state.kind === 'frozen_state'
? b.call('$.freeze', value)
: b.call('$.proxy', value)
: serialize_proxy_reassignment(value, private_state.id, state)
: value
);
}
Expand All @@ -240,7 +240,7 @@ export function serialize_set_binding(node, context, fallback, options) {
assignment.right =
public_state.kind === 'frozen_state'
? b.call('$.freeze', value)
: b.call('$.proxy', value);
: serialize_proxy_reassignment(value, public_state.id, state);
return assignment;
}
}
Expand Down Expand Up @@ -305,7 +305,7 @@ export function serialize_set_binding(node, context, fallback, options) {
context.state.analysis.runes &&
!options?.skip_proxy_and_freeze &&
should_proxy_or_freeze(value, context.state.scope)
? b.call('$.proxy', value)
? serialize_proxy_reassignment(value, left_name, state)
: value
);
} else if (binding.kind === 'frozen_state') {
Expand Down Expand Up @@ -410,6 +410,25 @@ export function serialize_set_binding(node, context, fallback, options) {
return serialize();
}

/**
* @param {import('estree').Expression} value
* @param {import('estree').PrivateIdentifier | string} proxy_reference
* @param {import('./types').ClientTransformState} state
*/
export function serialize_proxy_reassignment(value, proxy_reference, state) {
return state.options.dev
? b.call(
'$.proxy',
value,
b.true,
b.null,
typeof proxy_reference === 'string'
? b.id(proxy_reference)
: b.member(b.this, proxy_reference)
)
: b.call('$.proxy', value);
}

/**
* @param {import('estree').ArrowFunctionExpression | import('estree').FunctionExpression} node
* @param {import('./types').ComponentContext} context
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,12 @@ import { get_rune } from '../../../scope.js';
import { is_hoistable_function, transform_inspect_rune } from '../../utils.js';
import * as b from '../../../../utils/builders.js';
import * as assert from '../../../../utils/assert.js';
import { get_prop_source, is_state_source, should_proxy_or_freeze } from '../utils.js';
import {
get_prop_source,
is_state_source,
serialize_proxy_reassignment,
should_proxy_or_freeze
} from '../utils.js';
import { extract_paths } from '../../../../utils/ast.js';
import { regex_invalid_identifier_chars } from '../../../patterns.js';

Expand Down Expand Up @@ -139,7 +144,11 @@ export const javascript_visitors_runes = {
'set',
definition.key,
[value],
[b.stmt(b.call('$.set', member, b.call('$.proxy', value)))]
[
b.stmt(
b.call('$.set', member, serialize_proxy_reassignment(value, field.id, state))
)
]
)
);
}
Expand Down
2 changes: 2 additions & 0 deletions packages/svelte/src/compiler/utils/builders.js
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,7 @@ export function do_while(test, body) {

const true_instance = literal(true);
const false_instance = literal(false);
const null_instane = literal(null);

/** @type {import('estree').DebuggerStatement} */
const debugger_builder = {
Expand Down Expand Up @@ -630,6 +631,7 @@ export {
return_builder as return,
if_builder as if,
this_instance as this,
null_instane as null,
debugger_builder as debugger
};

Expand Down
26 changes: 18 additions & 8 deletions packages/svelte/src/internal/client/proxy.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,10 @@ import * as e from './errors.js';
* @param {T} value
* @param {boolean} [immutable]
* @param {import('#client').ProxyMetadata | null} [parent]
* @param {import('#client').Source<T>} [prev] dev mode only
* @returns {import('#client').ProxyStateObject<T> | T}
*/
export function proxy(value, immutable = true, parent = null) {
export function proxy(value, immutable = true, parent = null, prev) {
if (typeof value === 'object' && value != null && !is_frozen(value)) {
// If we have an existing proxy, return it...
if (STATE_SYMBOL in value) {
Expand Down Expand Up @@ -71,13 +72,22 @@ export function proxy(value, immutable = true, parent = null) {
// @ts-expect-error
value[STATE_SYMBOL].parent = parent;

// @ts-expect-error
value[STATE_SYMBOL].owners =
parent === null
? current_component_context !== null
? new Set([current_component_context.function])
: null
: new Set();
if (prev) {
// Reuse owners from previous state; necessary because reassignment is not guaranteed to have correct component context.
// If no previous proxy exists we play it safe and assume ownerless state
// @ts-expect-error
const prev_owners = prev?.v?.[STATE_SYMBOL]?.owners;
// @ts-expect-error
value[STATE_SYMBOL].owners = prev_owners ? new Set(prev_owners) : null;
} else {
// @ts-expect-error
value[STATE_SYMBOL].owners =
parent === null
? current_component_context !== null
? new Set([current_component_context.function])
: null
: new Set();
}
}

return proxy;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
<script>
/** @type {{ object: { count: number }}} */
let { object = $bindable() } = $props();
let { object = $bindable(), reset } = $props();
</script>

<button onclick={() => object.count += 1}>
clicks: {object.count}
</button>
<button onclick={reset}>reset</button>
Original file line number Diff line number Diff line change
@@ -1,36 +1,30 @@
import { flushSync } from 'svelte';
import { test } from '../../test';

/** @type {typeof console.trace} */
let trace;

export default test({
html: `<button>clicks: 0</button>`,
html: `<button>clicks: 0</button> <button>reset</button>`,

compileOptions: {
dev: true
},

before_test: () => {
trace = console.trace;
console.trace = () => {};
},

after_test: () => {
console.trace = trace;
},

test({ assert, target, warnings }) {
const btn = target.querySelector('button');

flushSync(() => {
btn?.click();
});

assert.htmlEqual(target.innerHTML, `<button>clicks: 1</button>`);

assert.deepEqual(warnings, [
'Counter.svelte mutated a value owned by main.svelte. This is strongly discouraged. Consider passing values to child components with `bind:`, or use a callback instead'
]);
const warning =
'Counter.svelte mutated a value owned by main.svelte. This is strongly discouraged. Consider passing values to child components with `bind:`, or use a callback instead';
const [btn1, btn2] = target.querySelectorAll('button');

btn1.click();
flushSync();
assert.htmlEqual(target.innerHTML, `<button>clicks: 1</button> <button>reset</button>`);
assert.deepEqual(warnings, [warning]);

btn2.click();
flushSync();
assert.htmlEqual(target.innerHTML, `<button>clicks: 0</button> <button>reset</button>`);

btn1.click();
flushSync();
assert.htmlEqual(target.innerHTML, `<button>clicks: 1</button> <button>reset</button>`);
assert.deepEqual(warnings, [warning, warning]);
}
});
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<script>
import Counter from './Counter.svelte';

const object = $state({ count: 0 });
let object = $state({ count: 0 });
</script>

<Counter {object} />
<Counter {object} reset={() => object = {count: 0}} />
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { test } from '../../test';

export default test({
compileOptions: {
dev: true
},

async test({ assert, warnings }) {
assert.deepEqual(warnings, []);
}
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<script context="module">
let toast1 = $state();
let toast2 = $state({});

export async function show_toast() {
toast1 = {
message: 'foo',
show: true
};
toast1.show = false;

toast2 = {
message: 'foo',
show: true
};
toast2.show = false;
}
</script>
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<script>
import { show_toast } from "./child.svelte";

show_toast();
</script>
Loading