Skip to content

feat: compiler-driven each block reactivity #12744

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Aug 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ export function EachBlock(node, context) {
// evaluate expression in parent scope
context.visit(node.expression, {
...context.state,
expression: node.metadata.expression,
scope: /** @type {Scope} */ (context.state.scope.parent)
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,10 +115,10 @@ export function Identifier(node, context) {
}
}

if (binding && binding.kind !== 'normal') {
if (binding) {
if (context.state.expression) {
context.state.expression.dependencies.add(binding);
context.state.expression.has_state = true;
context.state.expression.has_state ||= binding.kind !== 'normal';
}

if (
Expand Down
14 changes: 0 additions & 14 deletions packages/svelte/src/compiler/phases/3-transform/client/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -269,20 +269,6 @@ export function should_proxy_or_freeze(node, scope) {
return true;
}

/**
* Port over the location information from the source to the target identifier.
* but keep the target as-is (i.e. a new id is created).
* This ensures esrap can generate accurate source maps.
* @param {Identifier} target
* @param {Identifier} source
*/
export function with_loc(target, source) {
if (source.loc) {
return { ...target, loc: source.loc };
}
return target;
}

/**
* @param {Pattern} node
* @param {import('zimmerframe').Context<SvelteNode, ComponentClientTransformState>} context
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
import { dev } from '../../../../state.js';
import { extract_paths, object } from '../../../../utils/ast.js';
import * as b from '../../../../utils/builders.js';
import { build_getter, with_loc } from '../utils.js';
import { build_getter } from '../utils.js';
import { get_value } from './shared/declarations.js';

/**
Expand Down Expand Up @@ -48,18 +48,26 @@ export function EachBlock(node, context) {
if (node.index) {
flags |= EACH_INDEX_REACTIVE;
}
}

// In runes mode, if key === item, we don't need to wrap the item in a source
const key_is_item =
/** @type {Expression} */ (node.key).type === 'Identifier' &&
node.context.type === 'Identifier' &&
node.context.name === node.key.name;
const key_is_item =
node.key?.type === 'Identifier' &&
node.context.type === 'Identifier' &&
node.context.name === node.key.name;

for (const binding of node.metadata.expression.dependencies) {
// if the expression doesn't reference any external state, we don't need to
// create a source for the item. TODO cover more cases (e.g. `x.filter(y)`
// should also qualify if `y` doesn't reference state, and non-state
// bindings should also be fine
if (binding.scope.function_depth >= context.state.scope.function_depth) {
continue;
}

if (!context.state.analysis.runes || !key_is_item) {
if (!context.state.analysis.runes || !key_is_item || binding.kind === 'store_sub') {
flags |= EACH_ITEM_REACTIVE;
break;
}
} else {
flags |= EACH_ITEM_REACTIVE;
}

// Since `animate:` can only appear on elements that are the sole child of a keyed each block,
Expand Down Expand Up @@ -134,21 +142,13 @@ export function EachBlock(node, context) {
const index =
each_node_meta.contains_group_binding || !node.index ? each_node_meta.index : b.id(node.index);
const item = each_node_meta.item;
const binding = /** @type {Binding} */ (context.state.scope.get(item.name));
const getter = (/** @type {Identifier} */ id) => {
const item_with_loc = with_loc(item, id);
return b.call('$.unwrap', item_with_loc);
};

if (node.index) {
child_state.transform[node.index] = {
read: (id) => {
const index_with_loc = with_loc(index, id);
return (flags & EACH_INDEX_REACTIVE) === 0
? index_with_loc
: b.call('$.get', index_with_loc);
}
};
if ((flags & EACH_INDEX_REACTIVE) !== 0) {
child_state.transform[node.index] = { read: get_value };
} else {
delete child_state.transform[node.index];
}

delete key_state.transform[node.index];
}
Expand All @@ -172,7 +172,7 @@ export function EachBlock(node, context) {

if (node.context.type === 'Identifier') {
child_state.transform[node.context.name] = {
read: getter,
read: (flags & EACH_ITEM_REACTIVE) !== 0 ? get_value : (node) => node,
assign: (_, value) => {
const left = b.member(
each_node_meta.array_name ? b.call(each_node_meta.array_name) : collection,
Expand All @@ -187,12 +187,12 @@ export function EachBlock(node, context) {

delete key_state.transform[node.context.name];
} else {
const unwrapped = getter(binding.node);
const paths = extract_paths(node.context);
const unwrapped = (flags & EACH_ITEM_REACTIVE) !== 0 ? b.call('$.get', item) : item;

for (const path of paths) {
for (const path of extract_paths(node.context)) {
const name = /** @type {Identifier} */ (path.node).name;
const needs_derived = path.has_default_value; // to ensure that default value is only called once

const fn = b.thunk(
/** @type {Expression} */ (context.visit(path.expression?.(unwrapped), child_state))
);
Expand All @@ -203,11 +203,11 @@ export function EachBlock(node, context) {

child_state.transform[name] = {
read,
assign: (node, value) => {
assign: (_, value) => {
const left = /** @type {Pattern} */ (path.update_expression(unwrapped));
return b.sequence([b.assignment('=', left, value), ...sequence]);
},
mutate: (node, mutation) => {
mutate: (_, mutation) => {
return b.sequence([mutation, ...sequence]);
}
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,11 @@ import { build_hoisted_params } from '../../utils.js';
export const visit_function = (node, context) => {
const metadata = node.metadata;

let state = context.state;
let state = { ...context.state, in_constructor: false };

if (node.type === 'FunctionExpression') {
const parent = /** @type {Node} */ (context.path.at(-1));
const in_constructor = parent.type === 'MethodDefinition' && parent.kind === 'constructor';

state = { ...context.state, in_constructor };
} else {
state = { ...context.state, in_constructor: false };
state.in_constructor = parent.type === 'MethodDefinition' && parent.kind === 'constructor';
}

if (metadata?.hoisted === true) {
Expand Down
2 changes: 2 additions & 0 deletions packages/svelte/src/compiler/phases/scope.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
/** @import { AnimateDirective, Binding, Component, DeclarationKind, EachBlock, ElementLike, LetDirective, SvelteComponent, SvelteNode, SvelteSelf, TransitionDirective, UseDirective } from '#compiler' */
import is_reference from 'is-reference';
import { walk } from 'zimmerframe';
import { create_expression_metadata } from './nodes.js';
import * as b from '../utils/builders.js';
import * as e from '../errors.js';
import {
Expand Down Expand Up @@ -574,6 +575,7 @@ export function create_scopes(ast, root, allow_reactive_declarations, parent) {
}

node.metadata = {
expression: create_expression_metadata(),
keyed: false,
contains_group_binding: false,
array_name: needs_array_deduplication ? state.scope.root.unique('$$array') : null,
Expand Down
1 change: 1 addition & 0 deletions packages/svelte/src/compiler/types/template.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,7 @@ export interface EachBlock extends BaseNode {
index?: string;
key?: Expression;
metadata: {
expression: ExpressionMetadata;
keyed: boolean;
contains_group_binding: boolean;
/** Set if something in the array expression is shadowed within the each block */
Expand Down
5 changes: 0 additions & 5 deletions packages/svelte/src/internal/client/dom/blocks/each.js
Original file line number Diff line number Diff line change
Expand Up @@ -149,11 +149,6 @@ export function each(node, flags, get_collection, get_key, render_fn, fallback_f
!(STATE_SYMBOL in array)
) {
flags ^= EACH_IS_STRICT_EQUALS;

// Additionally if we're in an keyed each block, we'll need ensure the items are all wrapped in signals.
if ((flags & EACH_KEYED) !== 0 && (flags & EACH_ITEM_REACTIVE) === 0) {
flags ^= EACH_ITEM_REACTIVE;
}
}

/** `true` if there was a hydration mismatch. Needs to be a `let` or else it isn't treeshaken out */
Expand Down
1 change: 0 additions & 1 deletion packages/svelte/src/internal/client/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,6 @@ export {
exclude_from_object,
pop,
push,
unwrap,
deep_read,
deep_read_state,
getAllContexts,
Expand Down
25 changes: 0 additions & 25 deletions packages/svelte/src/internal/client/runtime.js
Original file line number Diff line number Diff line change
Expand Up @@ -845,17 +845,6 @@ export function set_signal_status(signal, status) {
signal.f = (signal.f & STATUS_MASK) | status;
}

/**
* @template V
* @param {V | Value<V>} val
* @returns {val is Value<V>}
*/
export function is_signal(val) {
return (
typeof val === 'object' && val !== null && typeof (/** @type {Value<V>} */ (val).f) === 'number'
);
}

/**
* Retrieves the context that belongs to the closest parent component with the specified `key`.
* Must be called during component initialisation.
Expand Down Expand Up @@ -1139,20 +1128,6 @@ export function deep_read(value, visited = new Set()) {
}
}

/**
* @template V
* @param {V | Value<V>} value
* @returns {V}
*/
export function unwrap(value) {
if (is_signal(value)) {
// @ts-ignore
return get(value);
}
// @ts-ignore
return value;
}

if (DEV) {
/**
* @param {string} rune
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ export default function Each_string_template($$anchor) {
var fragment = $.comment();
var node = $.first_child(fragment);

$.each(node, 1, () => ['foo', 'bar', 'baz'], $.index, ($$anchor, thing, $$index) => {
$.each(node, 0, () => ['foo', 'bar', 'baz'], $.index, ($$anchor, thing, $$index) => {
var text = $.text();

$.template_effect(() => $.set_text(text, `${$.unwrap(thing) ?? ""}, `));
$.template_effect(() => $.set_text(text, `${thing ?? ""}, `));
$.append($$anchor, text);
});

Expand Down
1 change: 1 addition & 0 deletions packages/svelte/types/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1851,6 +1851,7 @@ declare module 'svelte/compiler' {
index?: string;
key?: Expression;
metadata: {
expression: ExpressionMetadata;
keyed: boolean;
contains_group_binding: boolean;
/** Set if something in the array expression is shadowed within the each block */
Expand Down
Loading