From 6a3e293207934b8f2deb88e2f56a001d9ffcebfb Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Thu, 20 Jun 2024 20:26:05 +0100 Subject: [PATCH] fix: wait a microtask for await blocks to reduce UI churn (#11989) * fix: wait a microtask for await blocks to reduce UI churn * fix: wait a microtask for await blocks to reduce UI churn * fix: wait a microtask for await blocks to reduce UI churn * fix bug * Make then blocks reactive * add test * update test * update test * Update packages/svelte/src/internal/client/dom/blocks/await.js Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com> * Add support for catch block * slightly more specific naming * if we use the reserved $$ prefix we dont need to mess around with scope.generate * omit args for then/catch if unnecessary * neaten up some old code * shrink code * simplify test * add failing test * preserve pending blocks * update test * fix comment typo * tidy up --------- Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com> Co-authored-by: Rich Harris --- .changeset/gentle-eagles-walk.md | 5 + .../phases/3-transform/client/utils.js | 42 +++++ .../3-transform/client/visitors/template.js | 76 +++++---- packages/svelte/src/compiler/phases/scope.js | 4 +- .../src/internal/client/dom/blocks/await.js | 146 ++++++++++-------- .../_config.js | 1 + .../await-then-no-expression/main.svelte | 2 +- .../_config.js | 38 ++--- .../svelte/tests/runtime-legacy/shared.ts | 4 +- .../await-pending-persistent/_config.js | 24 +++ .../await-pending-persistent/main.svelte | 17 ++ .../samples/await-resolve-2/_config.js | 21 +++ .../samples/await-resolve-2/main.svelte | 21 +++ .../samples/await-resolve/_config.js | 27 ++++ .../samples/await-resolve/main.svelte | 17 ++ 15 files changed, 320 insertions(+), 125 deletions(-) create mode 100644 .changeset/gentle-eagles-walk.md create mode 100644 packages/svelte/tests/runtime-runes/samples/await-pending-persistent/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/await-pending-persistent/main.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/await-resolve-2/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/await-resolve-2/main.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/await-resolve/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/await-resolve/main.svelte diff --git a/.changeset/gentle-eagles-walk.md b/.changeset/gentle-eagles-walk.md new file mode 100644 index 000000000..4ed6c5b3f --- /dev/null +++ b/.changeset/gentle-eagles-walk.md @@ -0,0 +1,5 @@ +--- +"svelte": patch +--- + +fix: wait a microtask for await blocks to reduce UI churn diff --git a/packages/svelte/src/compiler/phases/3-transform/client/utils.js b/packages/svelte/src/compiler/phases/3-transform/client/utils.js index 89a1a5000..884a7addf 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/utils.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/utils.js @@ -1,5 +1,6 @@ import * as b from '../../../utils/builders.js'; import { + extract_identifiers, extract_paths, is_expression_async, is_simple_expression, @@ -684,3 +685,44 @@ export function with_loc(target, source) { } return target; } + +/** + * @param {import("estree").Pattern} node + * @param {import("zimmerframe").Context} context + * @returns {{ id: import("estree").Pattern, declarations: null | import("estree").Statement[] }} + */ +export function create_derived_block_argument(node, context) { + if (node.type === 'Identifier') { + return { id: node, declarations: null }; + } + + const pattern = /** @type {import('estree').Pattern} */ (context.visit(node)); + const identifiers = extract_identifiers(node); + + const id = b.id('$$source'); + const value = b.id('$$value'); + + const block = b.block([ + b.var(pattern, b.call('$.get', id)), + b.return(b.object(identifiers.map((identifier) => b.prop('init', identifier, identifier)))) + ]); + + const declarations = [b.var(value, create_derived(context.state, b.thunk(block)))]; + + for (const id of identifiers) { + declarations.push( + b.var(id, create_derived(context.state, b.thunk(b.member(b.call('$.get', value), id)))) + ); + } + + return { id, declarations }; +} + +/** + * Svelte legacy mode should use safe equals in most places, runes mode shouldn't + * @param {import('./types.js').ComponentClientTransformState} state + * @param {import('estree').Expression} arg + */ +export function create_derived(state, arg) { + return b.call(state.analysis.runes ? '$.derived' : '$.derived_safe_equal', arg); +} 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 823b92415..e7d01f33d 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 @@ -21,7 +21,9 @@ import { function_visitor, get_assignment_value, serialize_get_binding, - serialize_set_binding + serialize_set_binding, + create_derived, + create_derived_block_argument } from '../utils.js'; import { AttributeAliases, @@ -646,15 +648,6 @@ function collect_parent_each_blocks(context) { ); } -/** - * Svelte legacy mode should use safe equals in most places, runes mode shouldn't - * @param {import('../types.js').ComponentClientTransformState} state - * @param {import('estree').Expression} arg - */ -function create_derived(state, arg) { - return b.call(state.analysis.runes ? '$.derived' : '$.derived_safe_equal', arg); -} - /** * @param {import('#compiler').Component | import('#compiler').SvelteComponent | import('#compiler').SvelteSelf} node * @param {string} component_name @@ -2594,6 +2587,45 @@ export const template_visitors = { AwaitBlock(node, context) { context.state.template.push(''); + let then_block; + let catch_block; + + if (node.then) { + /** @type {import('estree').Pattern[]} */ + const args = [b.id('$$anchor')]; + const block = /** @type {import('estree').BlockStatement} */ (context.visit(node.then)); + + if (node.value) { + const argument = create_derived_block_argument(node.value, context); + + args.push(argument.id); + + if (argument.declarations !== null) { + block.body.unshift(...argument.declarations); + } + } + + then_block = b.arrow(args, block); + } + + if (node.catch) { + /** @type {import('estree').Pattern[]} */ + const args = [b.id('$$anchor')]; + const block = /** @type {import('estree').BlockStatement} */ (context.visit(node.catch)); + + if (node.error) { + const argument = create_derived_block_argument(node.error, context); + + args.push(argument.id); + + if (argument.declarations !== null) { + block.body.unshift(...argument.declarations); + } + } + + catch_block = b.arrow(args, block); + } + context.state.init.push( b.stmt( b.call( @@ -2606,28 +2638,8 @@ export const template_visitors = { /** @type {import('estree').BlockStatement} */ (context.visit(node.pending)) ) : b.literal(null), - node.then - ? b.arrow( - node.value - ? [ - b.id('$$anchor'), - /** @type {import('estree').Pattern} */ (context.visit(node.value)) - ] - : [b.id('$$anchor')], - /** @type {import('estree').BlockStatement} */ (context.visit(node.then)) - ) - : b.literal(null), - node.catch - ? b.arrow( - node.error - ? [ - b.id('$$anchor'), - /** @type {import('estree').Pattern} */ (context.visit(node.error)) - ] - : [b.id('$$anchor')], - /** @type {import('estree').BlockStatement} */ (context.visit(node.catch)) - ) - : b.literal(null) + then_block, + catch_block ) ) ); diff --git a/packages/svelte/src/compiler/phases/scope.js b/packages/svelte/src/compiler/phases/scope.js index 850ac9a40..604c084ab 100644 --- a/packages/svelte/src/compiler/phases/scope.js +++ b/packages/svelte/src/compiler/phases/scope.js @@ -613,7 +613,7 @@ export function create_scopes(ast, root, allow_reactive_declarations, parent) { 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'); + then_scope.declare(id, 'derived', 'const'); value_scope.declare(id, 'normal', 'const'); } } @@ -627,7 +627,7 @@ export function create_scopes(ast, root, allow_reactive_declarations, parent) { 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'); + catch_scope.declare(id, 'derived', 'const'); error_scope.declare(id, 'normal', 'const'); } } diff --git a/packages/svelte/src/internal/client/dom/blocks/await.js b/packages/svelte/src/internal/client/dom/blocks/await.js index 8956631a6..692d3e745 100644 --- a/packages/svelte/src/internal/client/dom/blocks/await.js +++ b/packages/svelte/src/internal/client/dom/blocks/await.js @@ -7,111 +7,135 @@ import { set_current_reaction, set_dev_current_component_function } from '../../runtime.js'; -import { block, branch, destroy_effect, pause_effect } from '../../reactivity/effects.js'; -import { INERT } from '../../constants.js'; +import { block, branch, pause_effect, resume_effect } from '../../reactivity/effects.js'; import { DEV } from 'esm-env'; +import { queue_micro_task } from '../task.js'; +import { hydrating } from '../hydration.js'; +import { set, source } from '../../reactivity/sources.js'; + +const PENDING = 0; +const THEN = 1; +const CATCH = 2; /** * @template V * @param {Comment} anchor * @param {(() => Promise)} get_input * @param {null | ((anchor: Node) => void)} pending_fn - * @param {null | ((anchor: Node, value: V) => void)} then_fn + * @param {null | ((anchor: Node, value: import('#client').Source) => void)} then_fn * @param {null | ((anchor: Node, error: unknown) => void)} catch_fn * @returns {void} */ export function await_block(anchor, get_input, pending_fn, then_fn, catch_fn) { - const component_context = current_component_context; - /** @type {any} */ - let component_function; - if (DEV) { - component_function = component_context?.function ?? null; - } + var component_context = current_component_context; /** @type {any} */ - let input; + var component_function = DEV ? component_context?.function : null; + + /** @type {V | Promise} */ + var input; /** @type {import('#client').Effect | null} */ - let pending_effect; + var pending_effect; /** @type {import('#client').Effect | null} */ - let then_effect; + var then_effect; /** @type {import('#client').Effect | null} */ - let catch_effect; + var catch_effect; + + var input_source = source(/** @type {V} */ (undefined)); + var error_source = source(undefined); + var resolved = false; /** - * @param {(anchor: Comment, value: any) => void} fn - * @param {any} value + * @param {PENDING | THEN | CATCH} state + * @param {boolean} restore */ - function create_effect(fn, value) { - set_current_effect(effect); - set_current_reaction(effect); // TODO do we need both? - set_current_component_context(component_context); - if (DEV) { - set_dev_current_component_function(component_function); + function update(state, restore) { + resolved = true; + + if (restore) { + set_current_effect(effect); + set_current_reaction(effect); // TODO do we need both? + set_current_component_context(component_context); + if (DEV) set_dev_current_component_function(component_function); + } + + if (state === PENDING && pending_fn) { + if (pending_effect) resume_effect(pending_effect); + else pending_effect = branch(() => pending_fn(anchor)); + } + + if (state === THEN && then_fn) { + if (then_effect) resume_effect(then_effect); + else then_effect = branch(() => then_fn(anchor, input_source)); + } + + if (state === CATCH && catch_fn) { + if (catch_effect) resume_effect(catch_effect); + else catch_effect = branch(() => catch_fn(anchor, error_source)); + } + + if (state !== PENDING && pending_effect) { + pause_effect(pending_effect, () => (pending_effect = null)); } - var e = branch(() => fn(anchor, value)); - if (DEV) { - set_dev_current_component_function(null); + + if (state !== THEN && then_effect) { + pause_effect(then_effect, () => (then_effect = null)); + } + + if (state !== CATCH && catch_effect) { + pause_effect(catch_effect, () => (catch_effect = null)); } - set_current_component_context(null); - set_current_reaction(null); - set_current_effect(null); - // without this, the DOM does not update until two ticks after the promise, - // resolves which is unexpected behaviour (and somewhat irksome to test) - flush_sync(); + if (restore) { + if (DEV) set_dev_current_component_function(null); + set_current_component_context(null); + set_current_reaction(null); + set_current_effect(null); - return e; + // without this, the DOM does not update until two ticks after the promise + // resolves, which is unexpected behaviour (and somewhat irksome to test) + flush_sync(); + } } - const effect = block(() => { + var effect = block(() => { if (input === (input = get_input())) return; if (is_promise(input)) { - const promise = /** @type {Promise} */ (input); + var promise = input; - if (pending_fn) { - if (pending_effect && (pending_effect.f & INERT) === 0) { - destroy_effect(pending_effect); - } - - pending_effect = branch(() => pending_fn(anchor)); - } - - if (then_effect) pause_effect(then_effect); - if (catch_effect) pause_effect(catch_effect); + resolved = false; promise.then( (value) => { if (promise !== input) return; - if (pending_effect) pause_effect(pending_effect); - - if (then_fn) { - then_effect = create_effect(then_fn, value); - } + set(input_source, value); + update(THEN, true); }, (error) => { if (promise !== input) return; - if (pending_effect) pause_effect(pending_effect); - - if (catch_fn) { - catch_effect = create_effect(catch_fn, error); - } + set(error_source, error); + update(CATCH, true); } ); - } else { - if (pending_effect) pause_effect(pending_effect); - if (catch_effect) pause_effect(catch_effect); - if (then_fn) { - if (then_effect) { - destroy_effect(then_effect); + if (hydrating) { + if (pending_fn) { + pending_effect = branch(() => pending_fn(anchor)); } - - then_effect = branch(() => then_fn(anchor, input)); + } else { + // Wait a microtask before checking if we should show the pending state as + // the promise might have resolved by the next microtask. + queue_micro_task(() => { + if (!resolved) update(PENDING, true); + }); } + } else { + set(input_source, input); + update(THEN, false); } // Inert effects are proactively detached from the effect tree. Returning a noop diff --git a/packages/svelte/tests/runtime-legacy/samples/await-then-destruct-computed-props/_config.js b/packages/svelte/tests/runtime-legacy/samples/await-then-destruct-computed-props/_config.js index 734fcf6ae..5c873067e 100644 --- a/packages/svelte/tests/runtime-legacy/samples/await-then-destruct-computed-props/_config.js +++ b/packages/svelte/tests/runtime-legacy/samples/await-then-destruct-computed-props/_config.js @@ -22,6 +22,7 @@ export default test({ prop3: { prop7: 'seven' }, prop4: { prop10: 'ten' } })); + await Promise.resolve(); assert.htmlEqual( target.innerHTML, ` diff --git a/packages/svelte/tests/runtime-legacy/samples/await-then-no-expression/main.svelte b/packages/svelte/tests/runtime-legacy/samples/await-then-no-expression/main.svelte index fedc7cd2b..a48870ae6 100644 --- a/packages/svelte/tests/runtime-legacy/samples/await-then-no-expression/main.svelte +++ b/packages/svelte/tests/runtime-legacy/samples/await-then-no-expression/main.svelte @@ -20,4 +20,4 @@

the promise is pending

{:then}

the promise is resolved

-{/await} \ No newline at end of file +{/await} diff --git a/packages/svelte/tests/runtime-legacy/samples/transition-js-await-block-outros/_config.js b/packages/svelte/tests/runtime-legacy/samples/transition-js-await-block-outros/_config.js index 29537d6ff..1ce8a8502 100644 --- a/packages/svelte/tests/runtime-legacy/samples/transition-js-await-block-outros/_config.js +++ b/packages/svelte/tests/runtime-legacy/samples/transition-js-await-block-outros/_config.js @@ -118,9 +118,8 @@ export default test({ assert.htmlEqual( target.innerHTML, ` -

43

+

44

loading...

-

44

` ); @@ -159,9 +158,8 @@ export default test({ assert.htmlEqual( target.innerHTML, ` -

44

+

45

loading...

-

45

` ); @@ -169,9 +167,8 @@ export default test({ assert.htmlEqual( target.innerHTML, ` -

44

+

45

loading...

-

45

` ); @@ -183,10 +180,8 @@ export default test({ assert.htmlEqual( target.innerHTML, ` -

44

+

45

loading...

-

45

-

loading...

` ); @@ -195,10 +190,8 @@ export default test({ assert.htmlEqual( target.innerHTML, ` -

44

-

loading...

-

45

-

loading...

+

45

+

loading...

` ); @@ -207,11 +200,8 @@ export default test({ assert.htmlEqual( target.innerHTML, ` -

44

-

loading...

-

45

-

loading...

-

46

+

46

+

loading...

` ); @@ -219,20 +209,12 @@ export default test({ assert.htmlEqual( target.innerHTML, ` -

44

-

46

+

46

+

loading...

` ); raf.tick((time += 20)); - assert.htmlEqual( - target.innerHTML, - ` -

46

- ` - ); - - raf.tick((time += 70)); assert.htmlEqual( target.innerHTML, ` diff --git a/packages/svelte/tests/runtime-legacy/shared.ts b/packages/svelte/tests/runtime-legacy/shared.ts index d7b64302b..ed32d2f0c 100644 --- a/packages/svelte/tests/runtime-legacy/shared.ts +++ b/packages/svelte/tests/runtime-legacy/shared.ts @@ -183,7 +183,9 @@ async function run_test_variant( if (str.slice(0, i).includes('logs')) { // eslint-disable-next-line no-console - console.log = (...args) => logs.push(...args); + console.log = (...args) => { + logs.push(...args); + }; } if (str.slice(0, i).includes('hydrate')) { diff --git a/packages/svelte/tests/runtime-runes/samples/await-pending-persistent/_config.js b/packages/svelte/tests/runtime-runes/samples/await-pending-persistent/_config.js new file mode 100644 index 000000000..d01619e7e --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/await-pending-persistent/_config.js @@ -0,0 +1,24 @@ +import { test } from '../../test'; + +export default test({ + async test({ assert, target, logs }) { + const [b1, b2] = target.querySelectorAll('button'); + + b1.click(); + await Promise.resolve(); + assert.htmlEqual( + target.innerHTML, + `

pending

` + ); + + b2.click(); + await Promise.resolve(); + await Promise.resolve(); + assert.htmlEqual( + target.innerHTML, + `

pending

` + ); + + assert.deepEqual(logs, ['rendering pending block']); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/await-pending-persistent/main.svelte b/packages/svelte/tests/runtime-runes/samples/await-pending-persistent/main.svelte new file mode 100644 index 000000000..3d1fe303b --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/await-pending-persistent/main.svelte @@ -0,0 +1,17 @@ + + +{#await promise} + {console.log('rendering pending block')} +

pending

+{:then value} + {console.log('rendering then block')} +

then {value}

+{/await} + + + diff --git a/packages/svelte/tests/runtime-runes/samples/await-resolve-2/_config.js b/packages/svelte/tests/runtime-runes/samples/await-resolve-2/_config.js new file mode 100644 index 000000000..88f1b1e96 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/await-resolve-2/_config.js @@ -0,0 +1,21 @@ +import { test } from '../../test'; + +export default test({ + async test({ assert, target, logs }) { + const [b1, b2, b3, b4] = target.querySelectorAll('button'); + b1.click(); + await Promise.resolve(); + b2.click(); + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + b3.click(); + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + b4.click(); + await Promise.resolve(); + await Promise.resolve(); + assert.deepEqual(logs, ['pending', 'a', 'b', 'c', 'pending']); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/await-resolve-2/main.svelte b/packages/svelte/tests/runtime-runes/samples/await-resolve-2/main.svelte new file mode 100644 index 000000000..4edfa88f3 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/await-resolve-2/main.svelte @@ -0,0 +1,21 @@ + + +{#await current_promise} + {console.log('pending')} +{:then value} + {console.log(value)} +{:catch} + {console.log('error')} +{/await} + + + + + diff --git a/packages/svelte/tests/runtime-runes/samples/await-resolve/_config.js b/packages/svelte/tests/runtime-runes/samples/await-resolve/_config.js new file mode 100644 index 000000000..11a645673 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/await-resolve/_config.js @@ -0,0 +1,27 @@ +import { test } from '../../test'; + +export default test({ + async test({ assert, target, logs }) { + const [b1, b2] = target.querySelectorAll('button'); + b1.click(); + await Promise.resolve(); + assert.htmlEqual( + target.innerHTML, + `

then a

` + ); + b2.click(); + await Promise.resolve(); + assert.htmlEqual( + target.innerHTML, + `

then a

` + ); + await Promise.resolve(); + await Promise.resolve(); + assert.htmlEqual( + target.innerHTML, + `

then b

` + ); + + assert.deepEqual(logs, ['rendering pending block', 'rendering then block']); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/await-resolve/main.svelte b/packages/svelte/tests/runtime-runes/samples/await-resolve/main.svelte new file mode 100644 index 000000000..62f2db773 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/await-resolve/main.svelte @@ -0,0 +1,17 @@ + + +{#await promise} + {console.log('rendering pending block')} +

pending

+{:then value} + {console.log('rendering then block')} +

then {value}

+{/await} + + +