fix: re-run non-render-bound deriveds on the server (#17674)

I tried to bring #14977 up-to-date but it's slipped too far. Also, I
wanted to try a slightly different approach.

In this PR, deriveds are memoized if they're created during render — in
other words if you have something like this...

```svelte
<script>
  let thing = $derived(expensivelyComputeThing());
</script>
```

...`thing` will only be computed once. This seems correct since the
inputs should never change during render.

For deriveds created _outside_ render, we re-run the derived each time
it is accessed, which fixes #14954. This way, there's still _some_
overhead compared to how deriveds work in the browser (where they only
recompute when their dependencies have changed), but only in the rare
places where it is necessary.

There is one wrinkle: writable deriveds. On `main` these are just
regular old variables, which means they can be written to during render.
This PR currently preserves that behaviour, but I'm not sure it's
desirable. It prevents the values of non-render-bound deriveds from ever
updating, and makes no sense in the context of render-bound deriveds
since they shouldn't be changing during render _anyway_. So my
preference would be to disallow writes to deriveds on the server, but
I'm not sure if we would need to consider that a breaking change.

Draft because of that question, and also because I think we might be
able to tidy up some stuff around class fields.

- [x] figure out if we can delete some existing code around derived
class fields
- [x] figure out what to do about writable deriveds
- [x] add a test

### Before submitting the PR, please make sure you do the following

- [x] It's really useful if your PR references an issue where it is
discussed ahead of time. In many cases, features are absent for a
reason. For large changes, please create an RFC:
https://github.com/sveltejs/rfcs
- [x] Prefix your PR title with `feat:`, `fix:`, `chore:`, or `docs:`.
- [x] This message body should clearly illustrate what problems it
solves.
- [x] Ideally, include a test that fails without this PR but passes with
it.
- [x] If this PR changes code within `packages/svelte/src`, add a
changeset (`npx changeset`).

### Tests and linting

- [x] Run the tests with `pnpm test` and lint the project with `pnpm
lint`
pull/17742/head
Rich Harris 6 months ago committed by GitHub
parent be24b0dca7
commit 09c4cb5084
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194

@ -0,0 +1,5 @@
---
'svelte': patch
---
fix: re-run non-render-bound deriveds on the server

@ -68,35 +68,52 @@ function build_assignment(operator, left, right, context) {
object = object.object; object = object.object;
} }
if (object.type !== 'Identifier' || !is_store_name(object.name)) { if (object.type !== 'Identifier') {
return null; return null;
} }
const name = object.name.slice(1); if (is_store_name(object.name)) {
const name = object.name.slice(1);
if (!context.state.scope.get(name)) { if (!context.state.scope.get(name)) {
return null; return null;
}
if (object === left) {
let value = /** @type {Expression} */ (
context.visit(build_assignment_value(operator, left, right))
);
return b.call('$.store_set', b.id(name), value);
}
return b.call(
'$.store_mutate',
b.assignment('??=', b.id('$$store_subs'), b.object([])),
b.literal(object.name),
b.id(name),
b.assignment(
operator,
/** @type {Pattern} */ (context.visit(left)),
/** @type {Expression} */ (context.visit(right))
)
);
} }
if (object === left) { const binding = context.state.scope.get(object.name);
// TODO 6.0 this won't work perfectly: once a derived is written to, it will
// no longer recompute. It might be better to disallow writing to deriveds
// on the server, to prevent this bug occurring
if (binding?.kind === 'derived' && object === left) {
let value = /** @type {Expression} */ ( let value = /** @type {Expression} */ (
context.visit(build_assignment_value(operator, left, right)) context.visit(build_assignment_value(operator, left, right))
); );
return b.call('$.store_set', b.id(name), value); return b.call(binding.node, value);
} }
return b.call( return null;
'$.store_mutate',
b.assignment('??=', b.id('$$store_subs'), b.object([])),
b.literal(object.name),
b.id(name),
b.assignment(
operator,
/** @type {Pattern} */ (context.visit(left)),
/** @type {Expression} */ (context.visit(right))
)
);
} }
/** /**

@ -1,3 +1,4 @@
/** @import { Expression } from 'estree' */
/** @import { AST } from '#compiler' */ /** @import { AST } from '#compiler' */
/** @import { ComponentContext } from '../types.js' */ /** @import { ComponentContext } from '../types.js' */
import * as b from '#compiler/builders'; import * as b from '#compiler/builders';
@ -8,5 +9,5 @@ import { build_inline_component } from './shared/component.js';
* @param {ComponentContext} context * @param {ComponentContext} context
*/ */
export function Component(node, context) { export function Component(node, context) {
build_inline_component(node, b.id(node.name), context); build_inline_component(node, /** @type {Expression} */ (context.visit(b.id(node.name))), context);
} }

@ -14,6 +14,11 @@ export function Identifier(node, context) {
return b.id('$$sanitized_props'); return b.id('$$sanitized_props');
} }
if (node.name.startsWith('$$derived_array')) {
// terrible hack, but easier than adding new stuff to `context.state` for now
return b.call(node);
}
return build_getter(node, context.state); return build_getter(node, context.state);
} }
} }

@ -84,10 +84,52 @@ export function VariableDeclaration(node, context) {
const args = /** @type {CallExpression} */ (init).arguments; const args = /** @type {CallExpression} */ (init).arguments;
const value = args.length > 0 ? /** @type {Expression} */ (context.visit(args[0])) : b.void0; const value = args.length > 0 ? /** @type {Expression} */ (context.visit(args[0])) : b.void0;
if (rune === '$derived.by') { if (rune === '$derived' || rune === '$derived.by') {
declarations.push( const is_async =
b.declarator(/** @type {Pattern} */ (context.visit(declarator.id)), b.call(value)) rune === '$derived' &&
); context.state.analysis.async_deriveds.has(
/** @type {CallExpression} */ (declarator.init)
);
let init = is_async
? b.await(b.call('$.async_derived', b.thunk(value, true)))
: b.call('$.derived', rune === '$derived' ? b.thunk(value) : value);
if (declarator.id.type === 'Identifier') {
declarations.push(
b.declarator(/** @type {Pattern} */ (context.visit(declarator.id)), init)
);
} else {
const call = /** @type {CallExpression} */ (declarator.init);
let rhs = value;
if (rune !== '$derived' || call.arguments[0].type !== 'Identifier') {
const id = b.id(context.state.scope.generate('$$d'));
rhs = b.call(id);
declarations.push(b.declarator(id, init));
}
const { inserts, paths } = extract_paths(declarator.id, rhs);
for (const { id, value } of inserts) {
id.name = context.state.scope.generate('$$derived_array');
const expression = /** @type {Expression} */ (context.visit(b.thunk(value)));
const call = b.call('$.derived', expression);
declarations.push(b.declarator(id, call));
}
for (const path of paths) {
const expression = /** @type {Expression} */ (context.visit(path.expression));
const call = b.call('$.derived', b.thunk(expression));
declarations.push(b.declarator(path.node, call));
}
}
continue; continue;
} }
@ -96,13 +138,6 @@ export function VariableDeclaration(node, context) {
continue; continue;
} }
if (rune === '$derived') {
declarations.push(
b.declarator(/** @type {Pattern} */ (context.visit(declarator.id)), value)
);
continue;
}
declarations.push(...create_state_declarators(declarator, context.state.scope, value)); declarations.push(...create_state_declarators(declarator, context.state.scope, value));
} }
} else { } else {

@ -274,6 +274,10 @@ export function build_getter(node, state) {
); );
} }
if (binding.kind === 'derived') {
return (binding.declaration_kind === 'var' ? b.maybe_call : b.call)(binding.node);
}
return node; return node;
} }

@ -3,7 +3,6 @@
/** @import { Context as ServerContext } from '../server/types.js' */ /** @import { Context as ServerContext } from '../server/types.js' */
import { extract_paths, is_expression_async } from '../../../utils/ast.js'; import { extract_paths, is_expression_async } from '../../../utils/ast.js';
import * as b from '#compiler/builders'; import * as b from '#compiler/builders';
import { get_value } from '../client/visitors/shared/declarations.js';
/** /**
* @template {ClientContext | ServerContext} Context * @template {ClientContext | ServerContext} Context

@ -153,7 +153,6 @@ export {
safe_get, safe_get,
tick, tick,
untrack, untrack,
exclude_from_object,
deep_read, deep_read,
deep_read_state, deep_read_state,
active_effect active_effect
@ -171,7 +170,7 @@ export {
} from './dom/operations.js'; } from './dom/operations.js';
export { attr, clsx } from '../shared/attributes.js'; export { attr, clsx } from '../shared/attributes.js';
export { snapshot } from '../shared/clone.js'; export { snapshot } from '../shared/clone.js';
export { noop, fallback, to_array } from '../shared/utils.js'; export { noop, fallback, to_array, exclude_from_object } from '../shared/utils.js';
export { export {
invalid_default_snippet, invalid_default_snippet,
validate_dynamic_element_tag, validate_dynamic_element_tag,

@ -752,30 +752,6 @@ export function untrack(fn) {
} }
} }
/**
* @param {Record<string | symbol, unknown>} obj
* @param {Array<string | symbol>} keys
* @returns {Record<string | symbol, unknown>}
*/
export function exclude_from_object(obj, keys) {
/** @type {Record<string | symbol, unknown>} */
var result = {};
for (var key in obj) {
if (!keys.includes(key)) {
result[key] = obj[key];
}
}
for (var symbol of Object.getOwnPropertySymbols(obj)) {
if (Object.propertyIsEnumerable.call(obj, symbol) && !keys.includes(symbol)) {
result[symbol] = obj[symbol];
}
}
return result;
}
/** /**
* Possibly traverse an object and read all its properties so that they're all reactive in case this is `$state`. * Possibly traverse an object and read all its properties so that they're all reactive in case this is `$state`.
* Does only check first level of an object for performance reasons (heuristic should be good for 99% of all cases). * Does only check first level of an object for performance reasons (heuristic should be good for 99% of all cases).

@ -23,6 +23,7 @@ import {
} from '../../utils.js'; } from '../../utils.js';
import { Renderer } from './renderer.js'; import { Renderer } from './renderer.js';
import * as e from './errors.js'; import * as e from './errors.js';
import { ssr_context } from './context.js';
// https://html.spec.whatwg.org/multipage/syntax.html#attributes-2 // https://html.spec.whatwg.org/multipage/syntax.html#attributes-2
// https://infra.spec.whatwg.org/#noncharacter // https://infra.spec.whatwg.org/#noncharacter
@ -469,7 +470,7 @@ export { push_element, pop_element, validate_snippet_args } from './dev.js';
export { snapshot } from '../shared/clone.js'; export { snapshot } from '../shared/clone.js';
export { fallback, to_array } from '../shared/utils.js'; export { fallback, to_array, exclude_from_object } from '../shared/utils.js';
export { export {
invalid_default_snippet, invalid_default_snippet,
@ -486,17 +487,29 @@ export { escape_html as escape };
* @returns {(new_value?: T) => (T | void)} * @returns {(new_value?: T) => (T | void)}
*/ */
export function derived(fn) { export function derived(fn) {
const get_value = once(fn); // deriveds created during render are memoized,
/** // deriveds created outside (e.g. SvelteKit `page` stuff) are not
* @type {T | undefined} const get_value = ssr_context === null ? fn : once(fn);
*/
/** @type {T | undefined} */
let updated_value; let updated_value;
return function (new_value) { return function (new_value) {
if (arguments.length === 0) { if (arguments.length === 0) {
return updated_value ?? get_value(); return updated_value ?? get_value();
} }
updated_value = new_value; updated_value = new_value;
return updated_value; return updated_value;
}; };
} }
/**
* @template T
* @param {()=>T} fn
*/
export function async_derived(fn) {
return Promise.resolve(fn()).then((value) => {
return () => value;
});
}

@ -204,9 +204,14 @@ export class Renderer {
set_ssr_context(parent); set_ssr_context(parent);
if (result instanceof Promise) { if (result instanceof Promise) {
result.finally(() => {
set_ssr_context(null);
});
if (child.global.mode === 'sync') { if (child.global.mode === 'sync') {
e.await_invalid(); e.await_invalid();
} }
// just to avoid unhandled promise rejections -- we'll end up throwing in `collect_async` if something fails // just to avoid unhandled promise rejections -- we'll end up throwing in `collect_async` if something fails
result.catch(() => {}); result.catch(() => {});
child.promise = result; child.promise = result;
@ -621,24 +626,26 @@ export class Renderer {
* @returns {Renderer} * @returns {Renderer}
*/ */
static #open_render(mode, component, options) { static #open_render(mode, component, options) {
const renderer = new Renderer( var previous_context = ssr_context;
new SSRState(mode, options.idPrefix ? options.idPrefix + '-' : '', options.csp)
);
renderer.push(BLOCK_OPEN);
push();
if (options.context) /** @type {SSRContext} */ (ssr_context).c = options.context;
/** @type {SSRContext} */ (ssr_context).r = renderer;
// @ts-expect-error try {
component(renderer, options.props ?? {}); const renderer = new Renderer(
new SSRState(mode, options.idPrefix ? options.idPrefix + '-' : '', options.csp)
);
pop(); /** @type {SSRContext} */
const context = { p: null, c: options.context ?? null, r: renderer };
set_ssr_context(context);
renderer.push(BLOCK_CLOSE); renderer.push(BLOCK_OPEN);
// @ts-expect-error
component(renderer, options.props ?? {});
renderer.push(BLOCK_CLOSE);
return renderer; return renderer;
} finally {
set_ssr_context(previous_context);
}
} }
/** /**

@ -118,3 +118,27 @@ export function to_array(value, n) {
return array; return array;
} }
/**
* @param {Record<string | symbol, unknown>} obj
* @param {Array<string | symbol>} keys
* @returns {Record<string | symbol, unknown>}
*/
export function exclude_from_object(obj, keys) {
/** @type {Record<string | symbol, unknown>} */
var result = {};
for (var key in obj) {
if (!keys.includes(key)) {
result[key] = obj[key];
}
}
for (var symbol of Object.getOwnPropertySymbols(obj)) {
if (Object.propertyIsEnumerable.call(obj, symbol) && !keys.includes(symbol)) {
result[symbol] = obj[symbol];
}
}
return result;
}

@ -13,6 +13,7 @@ import type { CompileOptions } from '#compiler';
import { suite_with_variants, type BaseTest } from '../suite.js'; import { suite_with_variants, type BaseTest } from '../suite.js';
import { clear } from '../../src/internal/client/reactivity/batch.js'; import { clear } from '../../src/internal/client/reactivity/batch.js';
import { hydrating } from '../../src/internal/client/dom/hydration.js'; import { hydrating } from '../../src/internal/client/dom/hydration.js';
import { ssr_context } from '../../src/internal/server/context.js';
type Assert = typeof import('vitest').assert & { type Assert = typeof import('vitest').assert & {
htmlEqual(a: string, b: string, description?: string): void; htmlEqual(a: string, b: string, description?: string): void;
@ -358,6 +359,10 @@ async function run_test_variant(
let snapshot = undefined; let snapshot = undefined;
if (variant === 'hydrate' || variant === 'ssr' || variant === 'async-ssr') { if (variant === 'hydrate' || variant === 'ssr' || variant === 'async-ssr') {
if (ssr_context !== null) {
throw new Error('ssr_context was not cleared');
}
config.before_test?.(); config.before_test?.();
// ssr into target // ssr into target
const SsrSvelteComponent = (await import(`${cwd}/_output/server/main.svelte.js`)).default; const SsrSvelteComponent = (await import(`${cwd}/_output/server/main.svelte.js`)).default;
@ -387,6 +392,10 @@ async function run_test_variant(
snapshot = config.snapshot(target); snapshot = config.snapshot(target);
} }
} }
if (ssr_context !== null) {
throw new Error('ssr_context was not cleared');
}
} else { } else {
target.innerHTML = ''; target.innerHTML = '';
} }

@ -0,0 +1,11 @@
import { test } from '../../test';
export default test({
test_ssr({ assert, logs }) {
assert.deepEqual(logs, [0, 2, { count: 2 }, 0, 0, { local_count: 1 }]);
},
test({ assert, logs }) {
assert.deepEqual(logs, [0, 2, { count: 2 }, 0, 0, { local_count: 1 }]);
}
});

@ -0,0 +1,21 @@
<script>
import { reset, increment, get, count } from './state.svelte.js';
reset();
console.log(get());
increment();
console.log(get());
console.log({ count });
let local_count = 0;
let s = $state(0);
let d = $derived.by(() => {
local_count += 1;
return s * 2;
});
console.log(d);
console.log(d);
console.log({ local_count });
</script>

@ -0,0 +1,20 @@
let s = $state(0);
let d = $derived.by(() => {
count += 1;
return s * 2;
});
export let count = 0;
export function reset() {
count = 0;
s = 0;
}
export function increment() {
s += 1;
}
export function get() {
return d;
}

@ -8,7 +8,7 @@ export default function Async_if_chain($$renderer) {
let foo = true; let foo = true;
var blocking; var blocking;
var $$promises = $$renderer.run([async () => blocking = await foo]); var $$promises = $$renderer.run([async () => blocking = await $.async_derived(() => foo)]);
$$renderer.async_block([$$promises[0]], ($$renderer) => { $$renderer.async_block([$$promises[0]], ($$renderer) => {
if (foo) { if (foo) {
@ -94,10 +94,10 @@ export default function Async_if_chain($$renderer) {
$$renderer.push(`<!--]--> `); $$renderer.push(`<!--]--> `);
$$renderer.async_block([$$promises[0]], ($$renderer) => { $$renderer.async_block([$$promises[0]], ($$renderer) => {
if (blocking > 10) { if (blocking() > 10) {
$$renderer.push('<!--[-->'); $$renderer.push('<!--[-->');
$$renderer.push(`foo`); $$renderer.push(`foo`);
} else if (blocking > 5) { } else if (blocking() > 5) {
$$renderer.push('<!--[1-->'); $$renderer.push('<!--[1-->');
$$renderer.push(`bar`); $$renderer.push(`bar`);
} else { } else {

@ -6,15 +6,15 @@ export default function Async_in_derived($$renderer, $$props) {
var yes1, yes2, no1, no2; var yes1, yes2, no1, no2;
var $$promises = $$renderer.run([ var $$promises = $$renderer.run([
async () => yes1 = await 1, async () => yes1 = await $.async_derived(() => 1),
async () => yes2 = foo(await 1), async () => yes2 = await $.async_derived(async () => foo(await 1)),
() => no1 = (async () => { () => no1 = $.derived(async () => {
return await 1; return await 1;
})(), }),
() => no2 = async () => { () => no2 = $.derived(() => async () => {
return await 1; return await 1;
} })
]); ]);
if (true) { if (true) {

@ -2,13 +2,13 @@ import * as $ from 'svelte/internal/server';
export default function Await_block_scope($$renderer) { export default function Await_block_scope($$renderer) {
let counter = { count: 0 }; let counter = { count: 0 };
const promise = Promise.resolve(counter); const promise = $.derived(() => Promise.resolve(counter));
function increment() { function increment() {
counter.count += 1; counter.count += 1;
} }
$$renderer.push(`<button>clicks: ${$.escape(counter.count)}</button> `); $$renderer.push(`<button>clicks: ${$.escape(counter.count)}</button> `);
$.await($$renderer, promise, () => {}, (counter) => {}); $.await($$renderer, promise(), () => {}, (counter) => {});
$$renderer.push(`<!--]--> ${$.escape(counter.count)}`); $$renderer.push(`<!--]--> ${$.escape(counter.count)}`);
} }
Loading…
Cancel
Save