From 2ce2b7d98b66e34c794b290ac0dc7135a4a3bc2b Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Fri, 19 Jul 2024 07:24:08 -0400 Subject: [PATCH] feat: warn if binding to a non-reactive property (#12500) * feat: warn if binding to a non-reactive property * tweak --- .changeset/nasty-mayflies-smoke.md | 5 ++ .../messages/client-warnings/warnings.md | 6 ++ .../3-transform/client/visitors/template.js | 52 +++++++++++++++- packages/svelte/src/internal/client/index.js | 1 + .../svelte/src/internal/client/validate.js | 45 +++++++++++++- .../svelte/src/internal/client/warnings.js | 14 +++++ .../binding-property-static/Child.svelte | 5 ++ .../binding-property-static/_config.js | 16 +++++ .../binding-property-static/main.svelte | 61 +++++++++++++++++++ 9 files changed, 202 insertions(+), 3 deletions(-) create mode 100644 .changeset/nasty-mayflies-smoke.md create mode 100644 packages/svelte/tests/runtime-runes/samples/binding-property-static/Child.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/binding-property-static/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/binding-property-static/main.svelte diff --git a/.changeset/nasty-mayflies-smoke.md b/.changeset/nasty-mayflies-smoke.md new file mode 100644 index 0000000000..495054744e --- /dev/null +++ b/.changeset/nasty-mayflies-smoke.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +feat: warn if binding to a non-reactive property diff --git a/packages/svelte/messages/client-warnings/warnings.md b/packages/svelte/messages/client-warnings/warnings.md index 2543a2065a..9ffef25b01 100644 --- a/packages/svelte/messages/client-warnings/warnings.md +++ b/packages/svelte/messages/client-warnings/warnings.md @@ -1,3 +1,9 @@ +## binding_property_non_reactive + +> `%binding%` is binding to a non-reactive property + +> `%binding%` (%location%) is binding to a non-reactive property + ## hydration_attribute_changed > The `%attribute%` attribute on `%html%` changed its value between server and client renders. The client value, `%value%`, will be ignored in favour of the server value diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js index cab004bf9f..645895144f 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js @@ -1,4 +1,6 @@ /** @import { BlockStatement, CallExpression, Expression, ExpressionStatement, Identifier, Literal, MemberExpression, ObjectExpression, Pattern, Property, Statement, Super, TemplateElement, TemplateLiteral } from 'estree' */ +/** @import { BindDirective } from '#compiler' */ +/** @import { ComponentClientTransformState } from '../types' */ import { extract_identifiers, extract_paths, @@ -776,11 +778,15 @@ function serialize_inline_component(node, component_name, context, anchor = cont push_prop(b.init(attribute.name, value)); } } else if (attribute.type === 'BindDirective') { + const expression = /** @type {Expression} */ (context.visit(attribute.expression)); + + if (expression.type === 'MemberExpression' && context.state.options.dev) { + context.state.init.push(serialize_validate_binding(context.state, attribute, expression)); + } + if (attribute.name === 'this') { bind_this = attribute.expression; } else { - const expression = /** @type {Expression} */ (context.visit(attribute.expression)); - if (context.state.options.dev) { binding_initializers.push( b.stmt(b.call(b.id('$.add_owner_effect'), b.thunk(expression), b.id(component_name))) @@ -2824,6 +2830,17 @@ export const template_visitors = { BindDirective(node, context) { const { state, path, visit } = context; const expression = node.expression; + + if (expression.type === 'MemberExpression' && context.state.options.dev) { + context.state.init.push( + serialize_validate_binding( + context.state, + node, + /**@type {MemberExpression} */ (visit(expression)) + ) + ); + } + const getter = b.thunk(/** @type {Expression} */ (visit(expression))); const assignment = b.assignment('=', expression, b.id('$$value')); const setter = b.arrow( @@ -3230,3 +3247,34 @@ export const template_visitors = { CallExpression: javascript_visitors_runes.CallExpression, VariableDeclaration: javascript_visitors_runes.VariableDeclaration }; + +/** + * @param {import('../types.js').ComponentClientTransformState} state + * @param {BindDirective} binding + * @param {MemberExpression} expression + */ +function serialize_validate_binding(state, binding, expression) { + const string = state.analysis.source.slice(binding.start, binding.end); + + const get_object = b.thunk(/** @type {Expression} */ (expression.object)); + const get_property = b.thunk( + /** @type {Expression} */ ( + expression.computed + ? expression.property + : b.literal(/** @type {Identifier} */ (expression.property).name) + ) + ); + + const loc = locator(binding.start); + + return b.stmt( + b.call( + '$.validate_binding', + b.literal(string), + get_object, + get_property, + loc && b.literal(loc.line), + loc && b.literal(loc.column) + ) + ); +} diff --git a/packages/svelte/src/internal/client/index.js b/packages/svelte/src/internal/client/index.js index 60dd1d0aa2..5901ba1938 100644 --- a/packages/svelte/src/internal/client/index.js +++ b/packages/svelte/src/internal/client/index.js @@ -146,6 +146,7 @@ export { hasContext } from './runtime.js'; export { + validate_binding, validate_dynamic_component, validate_each_keys, validate_prop_bindings diff --git a/packages/svelte/src/internal/client/validate.js b/packages/svelte/src/internal/client/validate.js index 5c6f6a6553..415982a02b 100644 --- a/packages/svelte/src/internal/client/validate.js +++ b/packages/svelte/src/internal/client/validate.js @@ -1,7 +1,9 @@ -import { untrack } from './runtime.js'; +import { dev_current_component_function, untrack } from './runtime.js'; import { get_descriptor, is_array } from '../shared/utils.js'; import * as e from './errors.js'; import { FILENAME } from '../../constants.js'; +import { render_effect } from './reactivity/effects.js'; +import * as w from './warnings.js'; /** regex of all html void element names */ const void_element_names = @@ -89,3 +91,44 @@ export function validate_prop_bindings($$props, bindable, exports, component) { } } } + +/** + * @param {string} binding + * @param {() => Record} get_object + * @param {() => string} get_property + * @param {number} line + * @param {number} column + */ +export function validate_binding(binding, get_object, get_property, line, column) { + var warned = false; + + var filename = dev_current_component_function?.[FILENAME]; + + render_effect(() => { + if (warned) return; + + var object = get_object(); + var property = get_property(); + + var ran = false; + + // by making the (possibly false, but it would be an extreme edge case) assumption + // that a getter has a corresponding setter, we can determine if a property is + // reactive by seeing if this effect has dependencies + var effect = render_effect(() => { + if (ran) return; + + // eslint-disable-next-line @typescript-eslint/no-unused-expressions + object[property]; + }); + + ran = true; + + if (effect.deps === null) { + var location = filename && `${filename}:${line}:${column}`; + w.binding_property_non_reactive(binding, location); + + warned = true; + } + }); +} diff --git a/packages/svelte/src/internal/client/warnings.js b/packages/svelte/src/internal/client/warnings.js index e339cf1a8c..e1a6aa4325 100644 --- a/packages/svelte/src/internal/client/warnings.js +++ b/packages/svelte/src/internal/client/warnings.js @@ -5,6 +5,20 @@ import { DEV } from 'esm-env'; var bold = 'font-weight: bold'; var normal = 'font-weight: normal'; +/** + * `%binding%` (%location%) is binding to a non-reactive property + * @param {string} binding + * @param {string | undefined | null} [location] + */ +export function binding_property_non_reactive(binding, location) { + if (DEV) { + console.warn(`%c[svelte] binding_property_non_reactive\n%c${location ? `\`${binding}\` (${location}) is binding to a non-reactive property` : `\`${binding}\` is binding to a non-reactive property`}`, bold, normal); + } else { + // TODO print a link to the documentation + console.warn("binding_property_non_reactive"); + } +} + /** * The `%attribute%` attribute on `%html%` changed its value between server and client renders. The client value, `%value%`, will be ignored in favour of the server value * @param {string} attribute diff --git a/packages/svelte/tests/runtime-runes/samples/binding-property-static/Child.svelte b/packages/svelte/tests/runtime-runes/samples/binding-property-static/Child.svelte new file mode 100644 index 0000000000..a1f8e5ba93 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/binding-property-static/Child.svelte @@ -0,0 +1,5 @@ + + +

{value}

diff --git a/packages/svelte/tests/runtime-runes/samples/binding-property-static/_config.js b/packages/svelte/tests/runtime-runes/samples/binding-property-static/_config.js new file mode 100644 index 0000000000..67bdc497a4 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/binding-property-static/_config.js @@ -0,0 +1,16 @@ +import { test } from '../../test'; + +export default test({ + compileOptions: { + dev: true + }, + + async test({ assert, warnings }) { + assert.deepEqual(warnings, [ + `\`bind:value={pojo.value}\` (main.svelte:50:7) is binding to a non-reactive property`, + `\`bind:value={frozen.value}\` (main.svelte:51:7) is binding to a non-reactive property`, + `\`bind:value={pojo.value}\` (main.svelte:52:7) is binding to a non-reactive property`, + `\`bind:value={frozen.value}\` (main.svelte:53:7) is binding to a non-reactive property` + ]); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/binding-property-static/main.svelte b/packages/svelte/tests/runtime-runes/samples/binding-property-static/main.svelte new file mode 100644 index 0000000000..c07062e3b2 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/binding-property-static/main.svelte @@ -0,0 +1,61 @@ + + + + + + + + + + + + + + +