fix: proxify values when assigning using `||=`, `&&=` and `??=` operators (#14273)

* add failing test for #14268

* simplify

* proxify values when using ||=, &&= and ??= assignment operators

* proxify values assigned to private state fields

* changeset

* fix

* fix

* add warning

* update test
pull/14537/head
Rich Harris 9 months ago committed by GitHub
parent bbee1fc7e0
commit fe0bd299a5
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194

@ -0,0 +1,5 @@
---
'svelte': patch
---
fix: proxify values when assigning using `||=`, `&&=` and `??=` operators

@ -1,5 +1,37 @@
<!-- This file is generated by scripts/process-messages/index.js. Do not edit! -->
### assignment_value_stale
```
Assignment to `%property%` property (%location%) will evaluate to the right-hand side, not the value of `%property%` following the assignment. This may result in unexpected behaviour.
```
Given a case like this...
```svelte
<script>
let object = $state({ array: null });
function add() {
(object.array ??= []).push(object.array.length);
}
</script>
<button onclick={add}>add</button>
<p>items: {JSON.stringify(object.items)}</p>
```
...the array being pushed to when the button is first clicked is the `[]` on the right-hand side of the assignment, but the resulting value of `object.array` is an empty state proxy. As a result, the pushed value will be discarded.
You can fix this by separating it into two statements:
```js
function add() {
object.array ??= [];
object.array.push(object.array.length);
}
```
### binding_property_non_reactive
```

@ -1,3 +1,33 @@
## assignment_value_stale
> Assignment to `%property%` property (%location%) will evaluate to the right-hand side, not the value of `%property%` following the assignment. This may result in unexpected behaviour.
Given a case like this...
```svelte
<script>
let object = $state({ array: null });
function add() {
(object.array ??= []).push(object.array.length);
}
</script>
<button onclick={add}>add</button>
<p>items: {JSON.stringify(object.items)}</p>
```
...the array being pushed to when the button is first clicked is the `[]` on the right-hand side of the assignment, but the resulting value of `object.array` is an empty state proxy. As a result, the pushed value will be discarded.
You can fix this by separating it into two statements:
```js
function add() {
object.array ??= [];
object.array.push(object.array.length);
}
```
## binding_property_non_reactive
> `%binding%` is binding to a non-reactive property

@ -1,8 +1,14 @@
/** @import { AssignmentExpression, AssignmentOperator, Expression, Pattern } from 'estree' */
/** @import { Location } from 'locate-character' */
/** @import { AssignmentExpression, AssignmentOperator, Expression, Identifier, Literal, MemberExpression, Pattern } from 'estree' */
/** @import { AST } from '#compiler' */
/** @import { Context } from '../types.js' */
import * as b from '../../../../utils/builders.js';
import { build_assignment_value } from '../../../../utils/ast.js';
import { is_ignored } from '../../../../state.js';
import {
build_assignment_value,
get_attribute_expression,
is_event_attribute
} from '../../../../utils/ast.js';
import { dev, filename, is_ignored, locator } from '../../../../state.js';
import { build_proxy_reassignment, should_proxy } from '../utils.js';
import { visit_assignment_expression } from '../../shared/assignments.js';
@ -20,6 +26,24 @@ export function AssignmentExpression(node, context) {
: expression;
}
/**
* Determines whether the value will be coerced on assignment (as with e.g. `+=`).
* If not, we may need to proxify the value, or warn that the value will not be
* proxified in time
* @param {AssignmentOperator} operator
*/
function is_non_coercive_operator(operator) {
return ['=', '||=', '&&=', '??='].includes(operator);
}
/** @type {Record<string, string>} */
const callees = {
'=': '$.assign',
'&&=': '$.assign_and',
'||=': '$.assign_or',
'??=': '$.assign_nullish'
};
/**
* @param {AssignmentOperator} operator
* @param {Pattern} left
@ -41,7 +65,11 @@ function build_assignment(operator, left, right, context) {
context.visit(build_assignment_value(operator, left, right))
);
if (private_state.kind !== 'raw_state' && should_proxy(value, context.state.scope)) {
if (
private_state.kind === 'state' &&
is_non_coercive_operator(operator) &&
should_proxy(value, context.state.scope)
) {
value = build_proxy_reassignment(value, b.member(b.this, private_state.id));
}
@ -73,24 +101,28 @@ function build_assignment(operator, left, right, context) {
? context.state.transform[object.name]
: null;
const path = context.path.map((node) => node.type);
// reassignment
if (object === left && transform?.assign) {
// special case — if an element binding, we know it's a primitive
const is_primitive = path.at(-1) === 'BindDirective' && path.at(-2) === 'RegularElement';
let value = /** @type {Expression} */ (
context.visit(build_assignment_value(operator, left, right))
);
// special case — if an element binding, we know it's a primitive
const path = context.path.map((node) => node.type);
const is_primitive = path.at(-1) === 'BindDirective' && path.at(-2) === 'RegularElement';
if (
!is_primitive &&
binding.kind !== 'prop' &&
binding.kind !== 'bindable_prop' &&
binding.kind !== 'raw_state' &&
context.state.analysis.runes &&
should_proxy(value, context.state.scope)
should_proxy(right, context.state.scope) &&
is_non_coercive_operator(operator)
) {
value = binding.kind === 'raw_state' ? value : build_proxy_reassignment(value, object);
value = build_proxy_reassignment(value, object);
}
return transform.assign(object, value);
@ -108,5 +140,57 @@ function build_assignment(operator, left, right, context) {
);
}
// in cases like `(object.items ??= []).push(value)`, we may need to warn
// if the value gets proxified, since the proxy _isn't_ the thing that
// will be pushed to. we do this by transforming it to something like
// `$.assign_nullish(object, 'items', [])`
let should_transform =
dev && path.at(-1) !== 'ExpressionStatement' && is_non_coercive_operator(operator);
// special case — ignore `onclick={() => (...)}`
if (
path.at(-1) === 'ArrowFunctionExpression' &&
(path.at(-2) === 'RegularElement' || path.at(-2) === 'SvelteElement')
) {
const element = /** @type {AST.RegularElement} */ (context.path.at(-2));
const attribute = element.attributes.find((attribute) => {
if (attribute.type !== 'Attribute' || !is_event_attribute(attribute)) {
return false;
}
const expression = get_attribute_expression(attribute);
return expression === context.path.at(-1);
});
if (attribute) {
should_transform = false;
}
}
if (left.type === 'MemberExpression' && should_transform) {
const callee = callees[operator];
const loc = /** @type {Location} */ (locator(/** @type {number} */ (left.start)));
const location = `${filename}:${loc.line}:${loc.column}`;
return /** @type {Expression} */ (
context.visit(
b.call(
callee,
/** @type {Expression} */ (left.object),
/** @type {Expression} */ (
left.computed
? left.property
: b.literal(/** @type {Identifier} */ (left.property).name)
),
right,
b.literal(location)
)
)
);
}
return null;
}

@ -0,0 +1,57 @@
import * as w from '../warnings.js';
import { sanitize_location } from './location.js';
/**
*
* @param {any} a
* @param {any} b
* @param {string} property
* @param {string} location
*/
function compare(a, b, property, location) {
if (a !== b) {
w.assignment_value_stale(property, /** @type {string} */ (sanitize_location(location)));
}
return a;
}
/**
* @param {any} object
* @param {string} property
* @param {any} value
* @param {string} location
*/
export function assign(object, property, value, location) {
return compare((object[property] = value), object[property], property, location);
}
/**
* @param {any} object
* @param {string} property
* @param {any} value
* @param {string} location
*/
export function assign_and(object, property, value, location) {
return compare((object[property] &&= value), object[property], property, location);
}
/**
* @param {any} object
* @param {string} property
* @param {any} value
* @param {string} location
*/
export function assign_or(object, property, value, location) {
return compare((object[property] ||= value), object[property], property, location);
}
/**
* @param {any} object
* @param {string} property
* @param {any} value
* @param {string} location
*/
export function assign_nullish(object, property, value, location) {
return compare((object[property] ??= value), object[property], property, location);
}

@ -1,4 +1,5 @@
export { FILENAME, HMR, NAMESPACE_SVG } from '../../constants.js';
export { assign, assign_and, assign_or, assign_nullish } from './dev/assign.js';
export { cleanup_styles } from './dev/css.js';
export { add_locations } from './dev/elements.js';
export { hmr } from './dev/hmr.js';

@ -5,6 +5,20 @@ import { DEV } from 'esm-env';
var bold = 'font-weight: bold';
var normal = 'font-weight: normal';
/**
* Assignment to `%property%` property (%location%) will evaluate to the right-hand side, not the value of `%property%` following the assignment. This may result in unexpected behaviour.
* @param {string} property
* @param {string} location
*/
export function assignment_value_stale(property, location) {
if (DEV) {
console.warn(`%c[svelte] assignment_value_stale\n%cAssignment to \`${property}\` property (${location}) will evaluate to the right-hand side, not the value of \`${property}\` following the assignment. This may result in unexpected behaviour.`, bold, normal);
} else {
// TODO print a link to the documentation
console.warn("assignment_value_stale");
}
}
/**
* `%binding%` (%location%) is binding to a non-reactive property
* @param {string} binding

@ -0,0 +1,24 @@
import { flushSync } from 'svelte';
import { test } from '../../test';
export default test({
compileOptions: {
dev: true
},
html: `<button>items: null</button>`,
test({ assert, target, warnings }) {
const btn = target.querySelector('button');
flushSync(() => btn?.click());
assert.htmlEqual(target.innerHTML, `<button>items: []</button>`);
flushSync(() => btn?.click());
assert.htmlEqual(target.innerHTML, `<button>items: [0]</button>`);
assert.deepEqual(warnings, [
'Assignment to `items` property (main.svelte:5:24) will evaluate to the right-hand side, not the value of `items` following the assignment. This may result in unexpected behaviour.'
]);
}
});

@ -0,0 +1,7 @@
<script>
let object = $state({ items: null });
</script>
<button onclick={() => (object.items ??= []).push(object.items.length)}>
items: {JSON.stringify(object.items)}
</button>

@ -0,0 +1,16 @@
import { flushSync } from 'svelte';
import { test } from '../../test';
export default test({
html: `<button>items: null</button>`,
test({ assert, target }) {
const [btn1, btn2] = target.querySelectorAll('button');
flushSync(() => btn1.click());
assert.htmlEqual(target.innerHTML, `<button>items: [0]</button>`);
flushSync(() => btn1.click());
assert.htmlEqual(target.innerHTML, `<button>items: [0,1]</button>`);
}
});

@ -0,0 +1,7 @@
<script>
let items = $state(null);
</script>
<button onclick={() => (items ??= []).push(items.length)}>
items: {JSON.stringify(items)}
</button>
Loading…
Cancel
Save