From 117082b0395f7b066b7396a04a3201e50638c667 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 19 Mar 2024 16:28:22 +0000 Subject: [PATCH 1/6] fix: improve handling of unowned derived signals (#10842) --- .changeset/olive-mice-fix.md | 5 ++ .../internal/client/reactivity/deriveds.js | 2 +- .../samples/derived-unowned-3/_config.js | 30 ++++++++++++ .../samples/derived-unowned-3/main.svelte | 49 +++++++++++++++++++ 4 files changed, 85 insertions(+), 1 deletion(-) create mode 100644 .changeset/olive-mice-fix.md create mode 100644 packages/svelte/tests/runtime-runes/samples/derived-unowned-3/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/derived-unowned-3/main.svelte diff --git a/.changeset/olive-mice-fix.md b/.changeset/olive-mice-fix.md new file mode 100644 index 0000000000..b3548bd11f --- /dev/null +++ b/.changeset/olive-mice-fix.md @@ -0,0 +1,5 @@ +--- +"svelte": patch +--- + +fix: improve handling of unowned derived signals diff --git a/packages/svelte/src/internal/client/reactivity/deriveds.js b/packages/svelte/src/internal/client/reactivity/deriveds.js index 24806084ae..b258e486ba 100644 --- a/packages/svelte/src/internal/client/reactivity/deriveds.js +++ b/packages/svelte/src/internal/client/reactivity/deriveds.js @@ -41,7 +41,7 @@ export function derived(fn) { /** @type {import('#client').DerivedDebug} */ (signal).inspect = new Set(); } - if (current_reaction !== null) { + if (current_reaction !== null && (current_reaction.f & DERIVED) !== 0) { if (current_reaction.deriveds === null) { current_reaction.deriveds = [signal]; } else { diff --git a/packages/svelte/tests/runtime-runes/samples/derived-unowned-3/_config.js b/packages/svelte/tests/runtime-runes/samples/derived-unowned-3/_config.js new file mode 100644 index 0000000000..3c1f70999f --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/derived-unowned-3/_config.js @@ -0,0 +1,30 @@ +import { flushSync } from 'svelte'; +import { test } from '../../test'; + +export default test({ + async test({ assert, target }) { + let [btn1, btn2] = target.querySelectorAll('button'); + + flushSync(() => { + btn1.click(); + }); + + flushSync(() => { + btn2.click(); + }); + + assert.htmlEqual( + target.innerHTML, + `\nneighba\nneighba` + ); + + flushSync(() => { + btn2.click(); + }); + + assert.htmlEqual( + target.innerHTML, + `\nzeeba\nzeeba` + ); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/derived-unowned-3/main.svelte b/packages/svelte/tests/runtime-runes/samples/derived-unowned-3/main.svelte new file mode 100644 index 0000000000..7c32ed0513 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/derived-unowned-3/main.svelte @@ -0,0 +1,49 @@ + + + + +{model.name} +{model.data.name} From 682f4a651351d4c0c492be1806bfb119b6bb76a7 Mon Sep 17 00:00:00 2001 From: Simon H <5968653+dummdidumm@users.noreply.github.com> Date: Tue, 19 Mar 2024 19:25:40 +0100 Subject: [PATCH 2/6] fix: adjust scope parent for named slots (#10843) fixes #10802 --- .changeset/mighty-cooks-scream.md | 5 +++++ packages/svelte/src/compiler/phases/scope.js | 6 +++--- .../samples/component-slot-let-scope-4/Nested.svelte | 1 + .../component-slot-let-scope-4/Nested2.svelte | 9 +++++++++ .../samples/component-slot-let-scope-4/_config.js | 7 +++++++ .../samples/component-slot-let-scope-4/main.svelte | 12 ++++++++++++ 6 files changed, 37 insertions(+), 3 deletions(-) create mode 100644 .changeset/mighty-cooks-scream.md create mode 100644 packages/svelte/tests/runtime-legacy/samples/component-slot-let-scope-4/Nested.svelte create mode 100644 packages/svelte/tests/runtime-legacy/samples/component-slot-let-scope-4/Nested2.svelte create mode 100644 packages/svelte/tests/runtime-legacy/samples/component-slot-let-scope-4/_config.js create mode 100644 packages/svelte/tests/runtime-legacy/samples/component-slot-let-scope-4/main.svelte diff --git a/.changeset/mighty-cooks-scream.md b/.changeset/mighty-cooks-scream.md new file mode 100644 index 0000000000..3d3e662fce --- /dev/null +++ b/.changeset/mighty-cooks-scream.md @@ -0,0 +1,5 @@ +--- +"svelte": patch +--- + +fix: adjust scope parent for named slots diff --git a/packages/svelte/src/compiler/phases/scope.js b/packages/svelte/src/compiler/phases/scope.js index cecd52bda8..9b0d3e7dcb 100644 --- a/packages/svelte/src/compiler/phases/scope.js +++ b/packages/svelte/src/compiler/phases/scope.js @@ -400,9 +400,9 @@ export function create_scopes(ast, root, allow_reactive_declarations, parent) { (attribute) => attribute.type === 'Attribute' && attribute.name === 'slot' ) ) { - //
inherits the scope above the component, because slots are hella weird - scopes.set(child, state.scope); - visit(child); + //
inherits the scope above the component unless the component is a named slot itself, because slots are hella weird + scopes.set(child, is_default_slot ? state.scope : scope); + visit(child, { scope: is_default_slot ? state.scope : scope }); } else { if (child.type === 'ExpressionTag') { // expression tag is a special case — we don't visit it directly, but via process_children, diff --git a/packages/svelte/tests/runtime-legacy/samples/component-slot-let-scope-4/Nested.svelte b/packages/svelte/tests/runtime-legacy/samples/component-slot-let-scope-4/Nested.svelte new file mode 100644 index 0000000000..5811f40c6a --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/component-slot-let-scope-4/Nested.svelte @@ -0,0 +1 @@ + diff --git a/packages/svelte/tests/runtime-legacy/samples/component-slot-let-scope-4/Nested2.svelte b/packages/svelte/tests/runtime-legacy/samples/component-slot-let-scope-4/Nested2.svelte new file mode 100644 index 0000000000..f4575dc01c --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/component-slot-let-scope-4/Nested2.svelte @@ -0,0 +1,9 @@ + + +
+ {text} +
+ +
\ No newline at end of file diff --git a/packages/svelte/tests/runtime-legacy/samples/component-slot-let-scope-4/_config.js b/packages/svelte/tests/runtime-legacy/samples/component-slot-let-scope-4/_config.js new file mode 100644 index 0000000000..18dd268ada --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/component-slot-let-scope-4/_config.js @@ -0,0 +1,7 @@ +import { test } from '../../test'; + +export default test({ + html: ` +
hello world
hello world
+ ` +}); diff --git a/packages/svelte/tests/runtime-legacy/samples/component-slot-let-scope-4/main.svelte b/packages/svelte/tests/runtime-legacy/samples/component-slot-let-scope-4/main.svelte new file mode 100644 index 0000000000..12d231dbbe --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/component-slot-let-scope-4/main.svelte @@ -0,0 +1,12 @@ + + + + +
+ {text} +
+
+
\ No newline at end of file From c564c771992708060b0b03715e42246407f916b7 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 19 Mar 2024 21:33:32 +0000 Subject: [PATCH 3/6] fix: ensure select value is updated upon select element removal (#10846) * fix: ensure select value is updated upon select element removal * lint * lol --- .changeset/smart-turkeys-tell.md | 5 +++ .../client/dom/elements/bindings/select.js | 22 ++++++++++++ .../binding-select-unmatched-3/_config.js | 34 +++++++++++++++++++ .../binding-select-unmatched-3/main.svelte | 12 +++++++ 4 files changed, 73 insertions(+) create mode 100644 .changeset/smart-turkeys-tell.md create mode 100644 packages/svelte/tests/runtime-legacy/samples/binding-select-unmatched-3/_config.js create mode 100644 packages/svelte/tests/runtime-legacy/samples/binding-select-unmatched-3/main.svelte diff --git a/.changeset/smart-turkeys-tell.md b/.changeset/smart-turkeys-tell.md new file mode 100644 index 0000000000..b4598ca14b --- /dev/null +++ b/.changeset/smart-turkeys-tell.md @@ -0,0 +1,5 @@ +--- +"svelte": patch +--- + +fix: ensure select value is updated upon select element removal diff --git a/packages/svelte/src/internal/client/dom/elements/bindings/select.js b/packages/svelte/src/internal/client/dom/elements/bindings/select.js index 5e6241f8cd..8c0f535cd1 100644 --- a/packages/svelte/src/internal/client/dom/elements/bindings/select.js +++ b/packages/svelte/src/internal/client/dom/elements/bindings/select.js @@ -91,6 +91,28 @@ export function bind_select_value(select, get_value, update) { select.__value = value; mounting = false; }); + + // If one of the options gets removed from the DOM, the value might have changed + effect(() => { + var observer = new MutationObserver(() => { + // @ts-ignore + var value = select.__value; + select_option(select, value, mounting); + /** @type {HTMLOptionElement | null} */ + var selected_option = select.querySelector(':checked'); + if (selected_option === null || get_option_value(selected_option) !== value) { + update(''); + } + }); + + observer.observe(select, { + childList: true + }); + + return () => { + observer.disconnect(); + }; + }); } /** diff --git a/packages/svelte/tests/runtime-legacy/samples/binding-select-unmatched-3/_config.js b/packages/svelte/tests/runtime-legacy/samples/binding-select-unmatched-3/_config.js new file mode 100644 index 0000000000..7c73cec9a2 --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/binding-select-unmatched-3/_config.js @@ -0,0 +1,34 @@ +import { ok, test } from '../../test'; + +// test select binding behavior when a selected option is removed +export default test({ + skip_if_ssr: 'permanent', + + html: `

selected: a

`, + + async test({ assert, component, target }) { + const select = target.querySelector('select'); + ok(select); + const options = target.querySelectorAll('option'); + + // first option should be selected by default since no value was bound + assert.equal(component.selected, 'a'); + assert.equal(select.value, 'a'); + assert.ok(options[0].selected); + + // remove the selected item, so the bound value no longer matches anything + component.items = ['b', 'c']; + + // There's a MutationObserver + await Promise.resolve(); + + // now no option should be selected + assert.equal(select.value, ''); + assert.equal(select.selectedIndex, -1); + + assert.htmlEqual( + target.innerHTML, + `

selected:

` + ); + } +}); diff --git a/packages/svelte/tests/runtime-legacy/samples/binding-select-unmatched-3/main.svelte b/packages/svelte/tests/runtime-legacy/samples/binding-select-unmatched-3/main.svelte new file mode 100644 index 0000000000..9cc18fa83b --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/binding-select-unmatched-3/main.svelte @@ -0,0 +1,12 @@ + + +

selected: {selected}

+ + From f5f9465edccb6c498964eb9f9b29f2914ef6713d Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 19 Mar 2024 22:23:09 +0000 Subject: [PATCH 4/6] feat: add support for webkitdirectory DOM boolean attribute (#10847) * feat: add support for webkitdirectory DOM boolean attribute * add to types --------- Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com> --- .changeset/beige-cobras-smoke.md | 5 +++++ packages/svelte/elements.d.ts | 1 + packages/svelte/src/constants.js | 3 ++- 3 files changed, 8 insertions(+), 1 deletion(-) create mode 100644 .changeset/beige-cobras-smoke.md diff --git a/.changeset/beige-cobras-smoke.md b/.changeset/beige-cobras-smoke.md new file mode 100644 index 0000000000..c3ec1ac04a --- /dev/null +++ b/.changeset/beige-cobras-smoke.md @@ -0,0 +1,5 @@ +--- +"svelte": patch +--- + +feat: add support for webkitdirectory DOM boolean attribute diff --git a/packages/svelte/elements.d.ts b/packages/svelte/elements.d.ts index 2dff32045d..6883aa0205 100644 --- a/packages/svelte/elements.d.ts +++ b/packages/svelte/elements.d.ts @@ -1056,6 +1056,7 @@ export interface HTMLInputAttributes extends HTMLAttributes { type?: HTMLInputTypeAttribute | undefined | null; value?: any; width?: number | string | undefined | null; + webkitdirectory?: boolean | undefined | null; 'on:change'?: ChangeEventHandler | undefined | null; onchange?: ChangeEventHandler | undefined | null; diff --git a/packages/svelte/src/constants.js b/packages/svelte/src/constants.js index 7996e3b2fa..1e7279c885 100644 --- a/packages/svelte/src/constants.js +++ b/packages/svelte/src/constants.js @@ -83,7 +83,8 @@ export const DOMBooleanAttributes = [ 'required', 'reversed', 'seamless', - 'selected' + 'selected', + 'webkitdirectory' ]; export const namespace_svg = 'http://www.w3.org/2000/svg'; From efe85fcce0c1066a03f69e64799ce514cd638839 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 20 Mar 2024 16:15:19 +0000 Subject: [PATCH 5/6] fix: more robust select element logic (#10848) * follow up to 10846 * lint * simplify * don't update value * rework logic, rely more on mutation observer, fix bug related to select multiple * fix lazy select options bug --------- Co-authored-by: Simon Holthausen Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com> Co-authored-by: brunnerh --- .changeset/smart-turkeys-tell.md | 2 +- .../3-transform/client/visitors/template.js | 204 +++++++++--------- .../client/dom/elements/bindings/select.js | 74 +++---- .../samples/binding-select-late-2/_config.js | 4 +- .../samples/binding-select-late-3/_config.js | 4 +- .../binding-select-unmatched-3/_config.js | 3 +- .../samples/select-lazy-options/_config.js | 16 ++ .../samples/select-lazy-options/main.svelte | 33 +++ .../_config.js | 21 ++ .../main.svelte | 9 + 10 files changed, 228 insertions(+), 142 deletions(-) create mode 100644 packages/svelte/tests/runtime-legacy/samples/select-lazy-options/_config.js create mode 100644 packages/svelte/tests/runtime-legacy/samples/select-lazy-options/main.svelte create mode 100644 packages/svelte/tests/runtime-legacy/samples/select-one-way-bind-object-multiple/_config.js create mode 100644 packages/svelte/tests/runtime-legacy/samples/select-one-way-bind-object-multiple/main.svelte diff --git a/.changeset/smart-turkeys-tell.md b/.changeset/smart-turkeys-tell.md index b4598ca14b..e864cd19df 100644 --- a/.changeset/smart-turkeys-tell.md +++ b/.changeset/smart-turkeys-tell.md @@ -2,4 +2,4 @@ "svelte": patch --- -fix: ensure select value is updated upon select element removal +fix: ensure select value is updated upon select option removal 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 e5302a3a7c..0d6f13aa18 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 @@ -158,26 +158,6 @@ function serialize_class_directives(class_directives, element_id, context, is_at } } -/** - * - * @param {string | null} spread_id - * @param {import('#compiler').RegularElement} node - * @param {import('../types.js').ComponentContext} context - * @param {import('estree').Identifier} node_id - */ -function add_select_to_spread_update(spread_id, node, context, node_id) { - if (spread_id !== null && node.name === 'select') { - context.state.update.push({ - grouped: b.if( - b.binary('in', b.literal('value'), b.id(spread_id)), - b.block([ - b.stmt(b.call('$.select_option', node_id, b.member(b.id(spread_id), b.id('value')))) - ]) - ) - }); - } -} - /** * @param {import('#compiler').Binding[]} references * @param {import('../types.js').ComponentContext} context @@ -223,6 +203,8 @@ function collect_transitive_dependencies(binding, seen = new Set()) { * @param {import('../types.js').ComponentContext} context */ function setup_select_synchronization(value_binding, context) { + if (context.state.analysis.runes) return; + let bound = value_binding.expression; while (bound.type === 'MemberExpression') { bound = /** @type {import('estree').Identifier | import('estree').MemberExpression} */ ( @@ -243,59 +225,49 @@ function setup_select_synchronization(value_binding, context) { } } - if (!context.state.analysis.runes) { - const invalidator = b.call( - '$.invalidate_inner_signals', - b.thunk( - b.block( - names.map((name) => { - const serialized = serialize_get_binding(b.id(name), context.state); - return b.stmt(serialized); - }) - ) + const invalidator = b.call( + '$.invalidate_inner_signals', + b.thunk( + b.block( + names.map((name) => { + const serialized = serialize_get_binding(b.id(name), context.state); + return b.stmt(serialized); + }) ) - ); + ) + ); - context.state.init.push( - b.stmt( - b.call( - '$.invalidate_effect', - b.thunk( - b.block([ - b.stmt( - /** @type {import('estree').Expression} */ (context.visit(value_binding.expression)) - ), - b.stmt(invalidator) - ]) - ) + context.state.init.push( + b.stmt( + b.call( + '$.invalidate_effect', + b.thunk( + b.block([ + b.stmt( + /** @type {import('estree').Expression} */ (context.visit(value_binding.expression)) + ), + b.stmt(invalidator) + ]) ) ) - ); - } + ) + ); } /** - * Serializes element attribute assignments that contain spreads to either only - * the init or the the init and update arrays, depending on whether or not the value is dynamic. - * Resulting code for static looks something like this: - * ```js - * $.spread_attributes(element, null, [...]); - * ``` - * Resulting code for dynamic looks something like this: - * ```js - * let value; - * $.render_effect(() => { - * value = $.spread_attributes(element, value, [...]) - * }); - * ``` - * Returns the id of the spread_attribute variable if spread isn't isolated, `null` otherwise. * @param {Array} attributes * @param {import('../types.js').ComponentContext} context * @param {import('#compiler').RegularElement} element * @param {import('estree').Identifier} element_id - * @returns {string | null} + * @param {boolean} needs_select_handling */ -function serialize_element_spread_attributes(attributes, context, element, element_id) { +function serialize_element_spread_attributes( + attributes, + context, + element, + element_id, + needs_select_handling +) { let needs_isolation = false; /** @type {import('estree').Expression[]} */ @@ -317,8 +289,9 @@ function serialize_element_spread_attributes(attributes, context, element, eleme const lowercase_attributes = element.metadata.svg || is_custom_element_node(element) ? b.false : b.true; + const id = context.state.scope.generate('spread_attributes'); - const isolated = b.stmt( + const standalone = b.stmt( b.call( '$.spread_attributes_effect', element_id, @@ -327,32 +300,57 @@ function serialize_element_spread_attributes(attributes, context, element, eleme b.literal(context.state.analysis.css.hash) ) ); + const inside_effect = b.stmt( + b.assignment( + '=', + b.id(id), + b.call( + '$.spread_attributes', + element_id, + b.id(id), + b.array(values), + lowercase_attributes, + b.literal(context.state.analysis.css.hash) + ) + ) + ); + + if (!needs_isolation || needs_select_handling) { + context.state.init.push(b.let(id)); + } // objects could contain reactive getters -> play it safe and always assume spread attributes are reactive if (needs_isolation) { - context.state.update_effects.push(isolated); - return null; + if (needs_select_handling) { + context.state.update_effects.push( + b.stmt(b.call('$.render_effect', b.arrow([], b.block([inside_effect])))) + ); + } else { + context.state.update_effects.push(standalone); + } } else { - const id = context.state.scope.generate('spread_attributes'); - context.state.init.push(b.let(id)); context.state.update.push({ - singular: isolated, - grouped: b.stmt( - b.assignment( - '=', - b.id(id), - b.call( - '$.spread_attributes', - element_id, - b.id(id), - b.array(values), - lowercase_attributes, - b.literal(context.state.analysis.css.hash) - ) - ) + singular: needs_select_handling ? undefined : standalone, + grouped: inside_effect + }); + } + + if (needs_select_handling) { + context.state.init.push( + b.stmt(b.call('$.init_select', element_id, b.thunk(b.member(b.id(id), b.id('value'))))) + ); + context.state.update.push({ + grouped: b.if( + b.binary('in', b.literal('value'), b.id(id)), + b.block([ + // This ensures a one-way street to the DOM in case it's . We need it in addition to $.init_select + // because the select value is not reflected as an attribute, so the + // mutation observer wouldn't notice. + b.stmt(b.call('$.select_option', element_id, b.member(b.id(id), b.id('value')))) + ]) ) }); - return id; } } @@ -644,27 +642,27 @@ function serialize_element_special_value_attribute(element, node_id, attribute, ) ); const is_reactive = attribute.metadata.dynamic; - const needs_selected_call = - element === 'option' && (is_reactive || collect_parent_each_blocks(context).length > 0); - const needs_option_call = element === 'select' && is_reactive; + const is_select_with_value = + // attribute.metadata.dynamic would give false negatives because even if the value does not change, + // the inner options could still change, so we need to always treat it as reactive + element === 'select' && attribute.value !== true && !is_text_attribute(attribute); const assignment = b.stmt( - needs_selected_call + is_select_with_value ? b.sequence([ inner_assignment, - // This ensures things stay in sync with the select binding - // in case of updates to the option value or new values appearing - b.call('$.selected', node_id) + // This ensures a one-way street to the DOM in case it's . We need it in addition to $.init_select + // because the select value is not reflected as an attribute, so the + // mutation observer wouldn't notice. + b.call('$.select_option', node_id, value) ]) - : needs_option_call - ? b.sequence([ - inner_assignment, - // This ensures a one-way street to the DOM in case it's - b.call('$.select_option', node_id, value) - ]) - : inner_assignment + : inner_assignment ); + if (is_select_with_value) { + state.init.push(b.stmt(b.call('$.init_select', node_id, b.thunk(value)))); + } + if (is_reactive) { const id = state.scope.generate(`${node_id.name}_value`); serialize_update_assignment( @@ -2082,11 +2080,15 @@ export const template_visitors = { // Then do attributes let is_attributes_reactive = false; if (node.metadata.has_spread) { - const spread_id = serialize_element_spread_attributes(attributes, context, node, node_id); - if (child_metadata.namespace !== 'foreign') { - add_select_to_spread_update(spread_id, node, context, node_id); - } - is_attributes_reactive = spread_id !== null; + serialize_element_spread_attributes( + attributes, + context, + node, + node_id, + // If value binding exists, that one takes care of calling $.init_select + value_binding === null && node.name === 'select' && child_metadata.namespace !== 'foreign' + ); + is_attributes_reactive = true; } else { for (const attribute of /** @type {import('#compiler').Attribute[]} */ (attributes)) { if (is_event_attribute(attribute)) { diff --git a/packages/svelte/src/internal/client/dom/elements/bindings/select.js b/packages/svelte/src/internal/client/dom/elements/bindings/select.js index 8c0f535cd1..6ee497f02b 100644 --- a/packages/svelte/src/internal/client/dom/elements/bindings/select.js +++ b/packages/svelte/src/internal/client/dom/elements/bindings/select.js @@ -1,4 +1,5 @@ import { effect } from '../../../reactivity/effects.js'; +import { untrack } from '../../../runtime.js'; /** * Selects the correct option(s) (depending on whether this is a multiple select) @@ -26,26 +27,43 @@ export function select_option(select, value, mounting) { } /** - * Finds the containing ` yet when this is called +export function init_select(select, get_value) { effect(() => { - var select = option.parentNode; - - while (select != null) { - if (select.nodeName === 'SELECT') break; - select = select.parentNode; + if (get_value) { + select_option(select, untrack(get_value)); } - // @ts-ignore - if (select != null && option.__value === select.__value) { - // never set to false, since this causes browser to select default option - option.selected = true; - } + var observer = new MutationObserver(() => { + // @ts-ignore + var value = select.__value; + select_option(select, value); + // Deliberately don't update the potential binding value, + // the model should be preserved unless explicitly changed + }); + + observer.observe(select, { + // Listen to option element changes + childList: true, + subtree: true, // because of + // Listen to option element value attribute changes + // (doesn't get notified of select value changes, + // because that property is not reflected as an attribute) + attributes: true, + attributeFilter: ['value'] + }); + + return () => { + observer.disconnect(); + }; }); } @@ -78,6 +96,7 @@ export function bind_select_value(select, get_value, update) { var value = get_value(); select_option(select, value, mounting); + // Mounting and value undefined -> take selection from dom if (mounting && value === undefined) { /** @type {HTMLOptionElement | null} */ var selected_option = select.querySelector(':checked'); @@ -92,27 +111,8 @@ export function bind_select_value(select, get_value, update) { mounting = false; }); - // If one of the options gets removed from the DOM, the value might have changed - effect(() => { - var observer = new MutationObserver(() => { - // @ts-ignore - var value = select.__value; - select_option(select, value, mounting); - /** @type {HTMLOptionElement | null} */ - var selected_option = select.querySelector(':checked'); - if (selected_option === null || get_option_value(selected_option) !== value) { - update(''); - } - }); - - observer.observe(select, { - childList: true - }); - - return () => { - observer.disconnect(); - }; - }); + // don't pass get_value, we already initialize it in the effect above + init_select(select); } /** diff --git a/packages/svelte/tests/runtime-legacy/samples/binding-select-late-2/_config.js b/packages/svelte/tests/runtime-legacy/samples/binding-select-late-2/_config.js index 15d0008fb6..ab2db4b07d 100644 --- a/packages/svelte/tests/runtime-legacy/samples/binding-select-late-2/_config.js +++ b/packages/svelte/tests/runtime-legacy/samples/binding-select-late-2/_config.js @@ -14,9 +14,11 @@ export default test({

selected: two

`, - test({ assert, component, target }) { + async test({ assert, component, target }) { component.items = ['one', 'two', 'three']; + await Promise.resolve(); // mutation observer + const options = target.querySelectorAll('option'); assert.ok(!options[0].selected); assert.ok(options[1].selected); diff --git a/packages/svelte/tests/runtime-legacy/samples/binding-select-late-3/_config.js b/packages/svelte/tests/runtime-legacy/samples/binding-select-late-3/_config.js index 1eec4b7dba..54773bea0a 100644 --- a/packages/svelte/tests/runtime-legacy/samples/binding-select-late-3/_config.js +++ b/packages/svelte/tests/runtime-legacy/samples/binding-select-late-3/_config.js @@ -19,9 +19,11 @@ export default test({

selected: two

`, - test({ assert, component, target }) { + async test({ assert, component, target }) { component.items = ['one', 'two', 'three']; + await Promise.resolve(); // mutation observer + const options = target.querySelectorAll('option'); assert.ok(!options[0].selected); assert.ok(options[1].selected); diff --git a/packages/svelte/tests/runtime-legacy/samples/binding-select-unmatched-3/_config.js b/packages/svelte/tests/runtime-legacy/samples/binding-select-unmatched-3/_config.js index 7c73cec9a2..05b485a066 100644 --- a/packages/svelte/tests/runtime-legacy/samples/binding-select-unmatched-3/_config.js +++ b/packages/svelte/tests/runtime-legacy/samples/binding-select-unmatched-3/_config.js @@ -26,9 +26,10 @@ export default test({ assert.equal(select.value, ''); assert.equal(select.selectedIndex, -1); + // model of selected value should be kept around, even if it is not in the list assert.htmlEqual( target.innerHTML, - `

selected:

` + `

selected: a

` ); } }); diff --git a/packages/svelte/tests/runtime-legacy/samples/select-lazy-options/_config.js b/packages/svelte/tests/runtime-legacy/samples/select-lazy-options/_config.js new file mode 100644 index 0000000000..03714f0875 --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/select-lazy-options/_config.js @@ -0,0 +1,16 @@ +import { test } from '../../test'; + +export default test({ + async test({ assert, target }) { + await target.querySelector('button')?.click(); + await Promise.resolve(); + + const options = target.querySelectorAll('option'); + assert.equal(options[0].selected, false); + assert.equal(options[1].selected, true); + assert.equal(options[2].selected, false); + assert.equal(options[3].selected, false); + assert.equal(options[4].selected, true); + assert.equal(options[5].selected, false); + } +}); diff --git a/packages/svelte/tests/runtime-legacy/samples/select-lazy-options/main.svelte b/packages/svelte/tests/runtime-legacy/samples/select-lazy-options/main.svelte new file mode 100644 index 0000000000..5d4b51c371 --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/select-lazy-options/main.svelte @@ -0,0 +1,33 @@ + + + + + + + diff --git a/packages/svelte/tests/runtime-legacy/samples/select-one-way-bind-object-multiple/_config.js b/packages/svelte/tests/runtime-legacy/samples/select-one-way-bind-object-multiple/_config.js new file mode 100644 index 0000000000..b737d42061 --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/select-one-way-bind-object-multiple/_config.js @@ -0,0 +1,21 @@ +import { test } from '../../test'; + +const items = [{ id: 'a' }, { id: 'b' }]; + +export default test({ + get props() { + return { foo: [items[0]], items }; + }, + + test({ assert, component, target }) { + const options = target.querySelectorAll('option'); + + assert.equal(options[0].selected, true); + assert.equal(options[1].selected, false); + + component.foo = [component.items[1]]; + + assert.equal(options[0].selected, false); + assert.equal(options[1].selected, true); + } +}); diff --git a/packages/svelte/tests/runtime-legacy/samples/select-one-way-bind-object-multiple/main.svelte b/packages/svelte/tests/runtime-legacy/samples/select-one-way-bind-object-multiple/main.svelte new file mode 100644 index 0000000000..830e1e74d5 --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/select-one-way-bind-object-multiple/main.svelte @@ -0,0 +1,9 @@ + + + From 86c57f96dec3587f6e5bc7e630349cec2e38c4bb Mon Sep 17 00:00:00 2001 From: Simon H <5968653+dummdidumm@users.noreply.github.com> Date: Wed, 20 Mar 2024 22:29:50 +0100 Subject: [PATCH 6/6] fix: better await block scope analysis (#10849) The await block scope analysis was flawed because it did not set the scope for the value/error field before visiting those nodes, resulting in the wrong scopes getting references As a byproduct, this fixes #8141 --- packages/svelte/src/compiler/phases/scope.js | 42 ++++++++++++------- .../samples/binding-select-late-4/_config.js | 12 ++++++ .../samples/binding-select-late-4/main.svelte | 19 +++++++++ 3 files changed, 58 insertions(+), 15 deletions(-) create mode 100644 packages/svelte/tests/runtime-legacy/samples/binding-select-late-4/_config.js create mode 100644 packages/svelte/tests/runtime-legacy/samples/binding-select-late-4/main.svelte diff --git a/packages/svelte/src/compiler/phases/scope.js b/packages/svelte/src/compiler/phases/scope.js index 9b0d3e7dcb..ad4fc2ee9e 100644 --- a/packages/svelte/src/compiler/phases/scope.js +++ b/packages/svelte/src/compiler/phases/scope.js @@ -599,26 +599,38 @@ export function create_scopes(ast, root, allow_reactive_declarations, parent) { }, AwaitBlock(node, context) { - context.next(); + context.visit(node.expression); + + if (node.pending) { + context.visit(node.pending); + } - if (node.then && node.value !== null) { - const then_scope = /** @type {Scope} */ (scopes.get(node.then)); - const value_scope = context.state.scope.child(); - for (const id of extract_identifiers(node.value)) { - then_scope.declare(id, 'normal', 'const'); - value_scope.declare(id, 'normal', 'const'); + if (node.then) { + context.visit(node.then); + if (node.value) { + const then_scope = /** @type {Scope} */ (scopes.get(node.then)); + const value_scope = context.state.scope.child(); + scopes.set(node.value, value_scope); + context.visit(node.value, { scope: value_scope }); + for (const id of extract_identifiers(node.value)) { + then_scope.declare(id, 'normal', 'const'); + value_scope.declare(id, 'normal', 'const'); + } } - scopes.set(node.value, value_scope); } - if (node.catch && node.error !== null) { - const catch_scope = /** @type {Scope} */ (scopes.get(node.catch)); - const error_scope = context.state.scope.child(); - for (const id of extract_identifiers(node.error)) { - catch_scope.declare(id, 'normal', 'const'); - error_scope.declare(id, 'normal', 'const'); + if (node.catch) { + context.visit(node.catch); + if (node.error) { + const catch_scope = /** @type {Scope} */ (scopes.get(node.catch)); + const error_scope = context.state.scope.child(); + scopes.set(node.error, error_scope); + context.visit(node.error, { scope: error_scope }); + for (const id of extract_identifiers(node.error)) { + catch_scope.declare(id, 'normal', 'const'); + error_scope.declare(id, 'normal', 'const'); + } } - scopes.set(node.error, error_scope); } }, diff --git a/packages/svelte/tests/runtime-legacy/samples/binding-select-late-4/_config.js b/packages/svelte/tests/runtime-legacy/samples/binding-select-late-4/_config.js new file mode 100644 index 0000000000..9893a7c716 --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/binding-select-late-4/_config.js @@ -0,0 +1,12 @@ +import { test } from '../../test'; + +export default test({ + async test({ assert, target }) { + await new Promise((r) => setTimeout(r, 200)); // wait for await block to resolve + + const options = target.querySelectorAll('option'); + assert.ok(!options[0].selected); + assert.ok(options[1].selected); + assert.ok(!options[2].selected); + } +}); diff --git a/packages/svelte/tests/runtime-legacy/samples/binding-select-late-4/main.svelte b/packages/svelte/tests/runtime-legacy/samples/binding-select-late-4/main.svelte new file mode 100644 index 0000000000..7702927e5b --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/binding-select-late-4/main.svelte @@ -0,0 +1,19 @@ + + +