From 6ed3ca380b4c86bc548b3163d4e85a4bccdcf613 Mon Sep 17 00:00:00 2001 From: Simon H <5968653+dummdidumm@users.noreply.github.com> Date: Wed, 9 Oct 2024 15:03:14 +0200 Subject: [PATCH] fix: make immutable option work more correctly (#13526) - make sure to not overfire before/afterUpdate - make sure to not fire mutable sources when they were mutated - only show deprecation warning when in runes mode to not clutter up console (this is in line with how we made it in other places) fixes #13454 --- .changeset/giant-ladybugs-thank.md | 5 +++ .../src/compiler/phases/2-analyze/index.js | 4 +- .../3-transform/client/transform-client.js | 8 +++- .../client/visitors/VariableDeclaration.js | 20 ++++++--- .../internal/client/dom/legacy/lifecycle.js | 36 ++++++++++++--- .../src/internal/client/reactivity/sources.js | 12 +++-- .../ImmutableTodo.svelte | 24 ++++++++++ .../immutable-before-after-update/_config.js | 45 +++++++++++++++++++ .../immutable-before-after-update/main.svelte | 26 +++++++++++ .../immutable-mutate-object/_config.js | 20 +++++++++ .../immutable-mutate-object/main.svelte | 6 +++ 11 files changed, 186 insertions(+), 20 deletions(-) create mode 100644 .changeset/giant-ladybugs-thank.md create mode 100644 packages/svelte/tests/runtime-legacy/samples/immutable-before-after-update/ImmutableTodo.svelte create mode 100644 packages/svelte/tests/runtime-legacy/samples/immutable-before-after-update/_config.js create mode 100644 packages/svelte/tests/runtime-legacy/samples/immutable-before-after-update/main.svelte create mode 100644 packages/svelte/tests/runtime-legacy/samples/immutable-mutate-object/_config.js create mode 100644 packages/svelte/tests/runtime-legacy/samples/immutable-mutate-object/main.svelte diff --git a/.changeset/giant-ladybugs-thank.md b/.changeset/giant-ladybugs-thank.md new file mode 100644 index 0000000000..832c2373cf --- /dev/null +++ b/.changeset/giant-ladybugs-thank.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: make immutable option work more correctly diff --git a/packages/svelte/src/compiler/phases/2-analyze/index.js b/packages/svelte/src/compiler/phases/2-analyze/index.js index 477fb9ca17..51365a10ec 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/index.js +++ b/packages/svelte/src/compiler/phases/2-analyze/index.js @@ -522,7 +522,7 @@ export function analyze_component(root, source, options) { if (root.options) { for (const attribute of root.options.attributes) { - if (attribute.name === 'accessors') { + if (attribute.name === 'accessors' && analysis.runes) { w.options_deprecated_accessors(attribute); } @@ -530,7 +530,7 @@ export function analyze_component(root, source, options) { w.options_missing_custom_element(attribute); } - if (attribute.name === 'immutable') { + if (attribute.name === 'immutable' && analysis.runes) { w.options_deprecated_immutable(attribute); } } diff --git a/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js b/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js index c98135f8e1..8369842e13 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js @@ -212,7 +212,9 @@ export function client_component(analysis, options) { for (const [name, binding] of analysis.instance.scope.declarations) { if (binding.kind === 'legacy_reactive') { - legacy_reactive_declarations.push(b.const(name, b.call('$.mutable_state'))); + legacy_reactive_declarations.push( + b.const(name, b.call('$.mutable_state', undefined, analysis.immutable ? b.true : undefined)) + ); } if (binding.kind === 'store_sub') { if (store_setup.length === 0) { @@ -368,7 +370,9 @@ export function client_component(analysis, options) { ...group_binding_declarations, ...analysis.top_level_snippets, .../** @type {ESTree.Statement[]} */ (instance.body), - analysis.runes || !analysis.needs_context ? b.empty : b.stmt(b.call('$.init')), + analysis.runes || !analysis.needs_context + ? b.empty + : b.stmt(b.call('$.init', analysis.immutable ? b.true : undefined)), .../** @type {ESTree.Statement[]} */ (template.body) ]); diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/VariableDeclaration.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/VariableDeclaration.js index af09af8b44..04685b66bd 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/VariableDeclaration.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/VariableDeclaration.js @@ -1,7 +1,6 @@ /** @import { CallExpression, Expression, Identifier, Literal, VariableDeclaration, VariableDeclarator } from 'estree' */ /** @import { Binding } from '#compiler' */ -/** @import { ComponentContext } from '../types' */ -/** @import { Scope } from '../../../scope' */ +/** @import { ComponentClientTransformState, ComponentContext } from '../types' */ import { dev } from '../../../../state.js'; import { extract_paths } from '../../../../utils/ast.js'; import * as b from '../../../../utils/builders.js'; @@ -267,7 +266,7 @@ export function VariableDeclaration(node, context) { declarations.push( ...create_state_declarators( declarator, - context.state.scope, + context.state, /** @type {Expression} */ (declarator.init && context.visit(declarator.init)) ) ); @@ -287,12 +286,17 @@ export function VariableDeclaration(node, context) { /** * Creates the output for a state declaration in legacy mode. * @param {VariableDeclarator} declarator - * @param {Scope} scope + * @param {ComponentClientTransformState} scope * @param {Expression} value */ -function create_state_declarators(declarator, scope, value) { +function create_state_declarators(declarator, { scope, analysis }, value) { if (declarator.id.type === 'Identifier') { - return [b.declarator(declarator.id, b.call('$.mutable_state', value))]; + return [ + b.declarator( + declarator.id, + b.call('$.mutable_state', value, analysis.immutable ? b.true : undefined) + ) + ]; } const tmp = scope.generate('tmp'); @@ -304,7 +308,9 @@ function create_state_declarators(declarator, scope, value) { const binding = scope.get(/** @type {Identifier} */ (path.node).name); return b.declarator( path.node, - binding?.kind === 'state' ? b.call('$.mutable_state', value) : value + binding?.kind === 'state' + ? b.call('$.mutable_state', value, analysis.immutable ? b.true : undefined) + : value ); }) ]; diff --git a/packages/svelte/src/internal/client/dom/legacy/lifecycle.js b/packages/svelte/src/internal/client/dom/legacy/lifecycle.js index 22d7e0feb4..5ffbacc670 100644 --- a/packages/svelte/src/internal/client/dom/legacy/lifecycle.js +++ b/packages/svelte/src/internal/client/dom/legacy/lifecycle.js @@ -1,21 +1,46 @@ /** @import { ComponentContextLegacy } from '#client' */ import { run, run_all } from '../../../shared/utils.js'; +import { derived } from '../../reactivity/deriveds.js'; import { user_pre_effect, user_effect } from '../../reactivity/effects.js'; import { component_context, deep_read_state, get, untrack } from '../../runtime.js'; /** * Legacy-mode only: Call `onMount` callbacks and set up `beforeUpdate`/`afterUpdate` effects + * @param {boolean} [immutable] */ -export function init() { +export function init(immutable = false) { const context = /** @type {ComponentContextLegacy} */ (component_context); const callbacks = context.l.u; if (!callbacks) return; + let props = () => deep_read_state(context.s); + + if (immutable) { + let version = 0; + let prev = /** @type {Record} */ ({}); + + // In legacy immutable mode, before/afterUpdate only fire if the object identity of a prop changes + const d = derived(() => { + let changed = false; + const props = context.s; + for (const key in props) { + if (props[key] !== prev[key]) { + prev[key] = props[key]; + changed = true; + } + } + if (changed) version++; + return version; + }); + + props = () => get(d); + } + // beforeUpdate if (callbacks.b.length) { user_pre_effect(() => { - observe_all(context); + observe_all(context, props); run_all(callbacks.b); }); } @@ -35,7 +60,7 @@ export function init() { // afterUpdate if (callbacks.a.length) { user_effect(() => { - observe_all(context); + observe_all(context, props); run_all(callbacks.a); }); } @@ -45,11 +70,12 @@ export function init() { * Invoke the getter of all signals associated with a component * so they can be registered to the effect this function is called in. * @param {ComponentContextLegacy} context + * @param {(() => void)} props */ -function observe_all(context) { +function observe_all(context, props) { if (context.l.s) { for (const signal of context.l.s) get(signal); } - deep_read_state(context.s); + props(); } diff --git a/packages/svelte/src/internal/client/reactivity/sources.js b/packages/svelte/src/internal/client/reactivity/sources.js index 978980ca76..b7b94afd5d 100644 --- a/packages/svelte/src/internal/client/reactivity/sources.js +++ b/packages/svelte/src/internal/client/reactivity/sources.js @@ -67,12 +67,15 @@ export function state(v) { /** * @template V * @param {V} initial_value + * @param {boolean} [immutable] * @returns {Source} */ /*#__NO_SIDE_EFFECTS__*/ -export function mutable_source(initial_value) { +export function mutable_source(initial_value, immutable = false) { const s = source(initial_value); - s.equals = safe_equals; + if (!immutable) { + s.equals = safe_equals; + } // bind the signal to the component context, in case we need to // track updates to trigger beforeUpdate/afterUpdate callbacks @@ -86,10 +89,11 @@ export function mutable_source(initial_value) { /** * @template V * @param {V} v + * @param {boolean} [immutable] * @returns {Source} */ -export function mutable_state(v) { - return push_derived_source(mutable_source(v)); +export function mutable_state(v, immutable = false) { + return push_derived_source(mutable_source(v, immutable)); } /** diff --git a/packages/svelte/tests/runtime-legacy/samples/immutable-before-after-update/ImmutableTodo.svelte b/packages/svelte/tests/runtime-legacy/samples/immutable-before-after-update/ImmutableTodo.svelte new file mode 100644 index 0000000000..7b88730481 --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/immutable-before-after-update/ImmutableTodo.svelte @@ -0,0 +1,24 @@ + + + + + diff --git a/packages/svelte/tests/runtime-legacy/samples/immutable-before-after-update/_config.js b/packages/svelte/tests/runtime-legacy/samples/immutable-before-after-update/_config.js new file mode 100644 index 0000000000..ed4b49ec1b --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/immutable-before-after-update/_config.js @@ -0,0 +1,45 @@ +import { flushSync } from 'svelte'; +import { test } from '../../test'; + +export default test({ + immutable: true, + + html: ' ', + + test({ assert, target, logs }) { + assert.deepEqual(logs, [ + '$:1', + 'beforeUpdate:1', + '$:2', + 'beforeUpdate:2', + '$:3', + 'beforeUpdate:3', + 'afterUpdate:1', + 'afterUpdate:2', + 'afterUpdate:3', + 'beforeUpdate:1', + 'beforeUpdate:2', + 'beforeUpdate:3' + ]); + + const [button1, button2] = target.querySelectorAll('button'); + + logs.length = 0; + button1.click(); + flushSync(); + assert.htmlEqual( + target.innerHTML, + ' ' + ); + assert.deepEqual(logs, ['$:1', 'beforeUpdate:1', 'afterUpdate:1']); + + logs.length = 0; + button2.click(); + flushSync(); + assert.htmlEqual( + target.innerHTML, + ' ' + ); + assert.deepEqual(logs, ['$:2', 'beforeUpdate:2', 'afterUpdate:2']); + } +}); diff --git a/packages/svelte/tests/runtime-legacy/samples/immutable-before-after-update/main.svelte b/packages/svelte/tests/runtime-legacy/samples/immutable-before-after-update/main.svelte new file mode 100644 index 0000000000..d24abfd113 --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/immutable-before-after-update/main.svelte @@ -0,0 +1,26 @@ + + +{#each todos as todo} + toggle(todo.id)} /> +{/each} diff --git a/packages/svelte/tests/runtime-legacy/samples/immutable-mutate-object/_config.js b/packages/svelte/tests/runtime-legacy/samples/immutable-mutate-object/_config.js new file mode 100644 index 0000000000..e3cbd48d75 --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/immutable-mutate-object/_config.js @@ -0,0 +1,20 @@ +import { flushSync } from 'svelte'; +import { test } from '../../test'; + +export default test({ + immutable: true, + + html: ' ', + + test({ assert, target }) { + const [button1, button2] = target.querySelectorAll('button'); + + button1.click(); + flushSync(); + assert.htmlEqual(target.innerHTML, ' '); + + button2.click(); + flushSync(); + assert.htmlEqual(target.innerHTML, ' '); + } +}); diff --git a/packages/svelte/tests/runtime-legacy/samples/immutable-mutate-object/main.svelte b/packages/svelte/tests/runtime-legacy/samples/immutable-mutate-object/main.svelte new file mode 100644 index 0000000000..8a086c5db3 --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/immutable-mutate-object/main.svelte @@ -0,0 +1,6 @@ + + + +