Skip to content
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

chore: set binding.kind before analysis #12843

Merged
merged 17 commits into from
Aug 14, 2024
5 changes: 5 additions & 0 deletions .changeset/pink-countries-repair.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

chore: set `binding.kind` before analysis
7 changes: 2 additions & 5 deletions packages/svelte/src/compiler/migrate/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -331,10 +331,7 @@ const instance_script = {

const binding = /** @type {Compiler.Binding} */ (state.scope.get(declarator.id.name));

if (
state.analysis.uses_props &&
(declarator.init || binding.mutated || binding.reassigned)
) {
if (state.analysis.uses_props && (declarator.init || binding.updated)) {
throw new Error(
'$$props is used together with named props in a way that cannot be automatically migrated.'
);
Expand All @@ -350,7 +347,7 @@ const instance_script = {
)
: '',
optional: !!declarator.init,
bindable: binding.mutated || binding.reassigned,
bindable: binding.updated,
...extract_type_and_comment(declarator, state.str, path)
});
state.props_insertion_point = /** @type {number} */ (declarator.end);
Expand Down
115 changes: 111 additions & 4 deletions packages/svelte/src/compiler/phases/2-analyze/index.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
/** @import { Node, Program } from 'estree' */
/** @import { Root, Script, SvelteNode, ValidatedCompileOptions, ValidatedModuleCompileOptions } from '#compiler' */
/** @import { Expression, Node, Program } from 'estree' */
/** @import { Binding, Root, Script, SvelteNode, ValidatedCompileOptions, ValidatedModuleCompileOptions } from '#compiler' */
/** @import { AnalysisState, Visitors } from './types' */
/** @import { Analysis, ComponentAnalysis, Js, ReactiveStatement, Template } from '../types' */
import { walk } from 'zimmerframe';
import * as e from '../../errors.js';
import * as w from '../../warnings.js';
import { is_text_attribute } from '../../utils/ast.js';
import { extract_identifiers, is_text_attribute } from '../../utils/ast.js';
import * as b from '../../utils/builders.js';
import { Scope, ScopeRoot, create_scopes, get_rune } from '../scope.js';
import { Scope, ScopeRoot, create_scopes, get_rune, set_scope } from '../scope.js';
import check_graph_for_cycles from './utils/check_graph_for_cycles.js';
import { create_attribute } from '../nodes.js';
import { analyze_css } from './css/css-analyze.js';
Expand Down Expand Up @@ -62,6 +62,7 @@ import { Text } from './visitors/Text.js';
import { TitleElement } from './visitors/TitleElement.js';
import { UpdateExpression } from './visitors/UpdateExpression.js';
import { VariableDeclarator } from './visitors/VariableDeclarator.js';
import is_reference from 'is-reference';

/**
* @type {Visitors}
Expand Down Expand Up @@ -397,6 +398,112 @@ export function analyze_component(root, source, options) {
source
};

if (!runes) {
// every exported `let` or `var` declaration becomes a prop, everything else becomes an export
for (const node of instance.ast.body) {
if (node.type !== 'ExportNamedDeclaration') continue;

analysis.needs_props = true;

if (node.declaration) {
if (
node.declaration.type === 'FunctionDeclaration' ||
node.declaration.type === 'ClassDeclaration'
) {
analysis.exports.push({
name: /** @type {import('estree').Identifier} */ (node.declaration.id).name,
alias: null
});
} else if (node.declaration.type === 'VariableDeclaration') {
if (node.declaration.kind === 'const') {
for (const declarator of node.declaration.declarations) {
for (const node of extract_identifiers(declarator.id)) {
analysis.exports.push({ name: node.name, alias: null });
}
}
} else {
for (const declarator of node.declaration.declarations) {
for (const id of extract_identifiers(declarator.id)) {
const binding = /** @type {Binding} */ (instance.scope.get(id.name));
binding.kind = 'bindable_prop';
}
}
}
}
} else {
for (const specifier of node.specifiers) {
const binding = instance.scope.get(specifier.local.name);

if (
binding &&
(binding.declaration_kind === 'var' || binding.declaration_kind === 'let')
) {
binding.kind = 'bindable_prop';

if (specifier.exported.name !== specifier.local.name) {
binding.prop_alias = specifier.exported.name;
}
} else {
analysis.exports.push({ name: specifier.local.name, alias: specifier.exported.name });
}
}
}
}

// if reassigned/mutated bindings are referenced in `$:` blocks
// or the template, turn them into state
for (const binding of instance.scope.declarations.values()) {
if (binding.kind !== 'normal') continue;

for (const { node, path } of binding.references) {
if (node === binding.node) continue;

if (binding.updated) {
if (
path[path.length - 1].type === 'StyleDirective' ||
path.some((node) => node.type === 'Fragment') ||
(path[1].type === 'LabeledStatement' && path[1].label.name === '$')
) {
binding.kind = 'state';
}
}
}
}

// more legacy nonsense: if an `each` binding is reassigned/mutated,
// treat the expression as being mutated as well
walk(/** @type {SvelteNode} */ (template.ast), null, {
EachBlock(node) {
const scope = /** @type {Scope} */ (template.scopes.get(node));

for (const binding of scope.declarations.values()) {
if (binding.updated) {
const state = { scope: /** @type {Scope} */ (scope.parent), scopes: template.scopes };

walk(node.expression, state, {
// @ts-expect-error
_: set_scope,
Identifier(node, context) {
const parent = /** @type {Expression} */ (context.path.at(-1));

if (is_reference(node, parent)) {
const binding = context.state.scope.get(node.name);

if (binding && binding.kind === 'normal') {
binding.kind = 'state';
binding.mutated = binding.updated = true;
}
}
}
});

break;
}
}
}
});
}

if (root.options) {
for (const attribute of root.options.attributes) {
if (attribute.name === 'accessors') {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ function get_delegated_event(event_name, handler, context) {
return unhoisted;
}

if (binding !== null && binding.initial !== null && !binding.mutated && !binding.is_called) {
if (binding !== null && binding.initial !== null && !binding.updated && !binding.is_called) {
const binding_type = binding.initial.type;

if (
Expand Down Expand Up @@ -188,7 +188,7 @@ function get_delegated_event(event_name, handler, context) {
(((!context.state.analysis.runes && binding.kind === 'each') ||
// or any normal not reactive bindings that are mutated.
binding.kind === 'normal') &&
binding.mutated))
binding.updated))
) {
return unhoisted;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export function BindDirective(node, context) {
binding.kind !== 'bindable_prop' &&
binding.kind !== 'each' &&
binding.kind !== 'store_sub' &&
!binding.mutated))
!binding.updated)) // TODO wut?
) {
e.bind_invalid_value(node.expression);
}
Expand Down Expand Up @@ -80,10 +80,6 @@ export function BindDirective(node, context) {
if (references.length > 0) {
parent.metadata.contains_group_binding = true;

for (const binding of parent.metadata.references) {
binding.mutated = true;
}

each_blocks.push(parent);
ids = ids.filter((id) => !references.includes(id));
ids.push(...extract_all_identifiers_from_expression(parent.expression)[1]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,37 +39,6 @@ export function ExportNamedDeclaration(node, context) {
}
}

if (context.state.ast_type === 'instance' && !context.state.analysis.runes) {
context.state.analysis.needs_props = true;

if (node.declaration) {
if (
node.declaration.type === 'FunctionDeclaration' ||
node.declaration.type === 'ClassDeclaration'
) {
context.state.analysis.exports.push({
name: /** @type {Identifier} */ (node.declaration.id).name,
alias: null
});
} else if (node.declaration.type === 'VariableDeclaration') {
if (node.declaration.kind === 'const') {
for (const declarator of node.declaration.declarations) {
for (const node of extract_identifiers(declarator.id)) {
context.state.analysis.exports.push({ name: node.name, alias: null });
}
}
} else {
for (const declarator of node.declaration.declarations) {
for (const id of extract_identifiers(declarator.id)) {
const binding = /** @type {Binding} */ (context.state.scope.get(id.name));
binding.kind = 'bindable_prop';
}
}
}
}
}
}

if (context.state.analysis.runes) {
if (node.declaration && context.state.ast_type === 'instance') {
if (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,29 +17,7 @@ export function ExportSpecifier(node, context) {
});

const binding = context.state.scope.get(node.local.name);
if (binding) binding.reassigned = true;
} else {
context.state.analysis.needs_props = true;

const binding = /** @type {Binding} */ (context.state.scope.get(node.local.name));

if (
binding !== null &&
(binding.kind === 'state' ||
binding.kind === 'raw_state' ||
(binding.kind === 'normal' &&
(binding.declaration_kind === 'let' || binding.declaration_kind === 'var')))
) {
binding.kind = 'bindable_prop';
if (node.exported.name !== node.local.name) {
binding.prop_alias = node.exported.name;
}
} else {
context.state.analysis.exports.push({
name: node.local.name,
alias: node.exported.name
});
}
if (binding) binding.reassigned = binding.updated = true;
}
} else {
validate_export(node, context.state.scope, node.local.name);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/** @import { Expression, Identifier } from 'estree' */
/** @import { EachBlock } from '#compiler' */
/** @import { Context } from '../types' */
import is_reference from 'is-reference';
import { should_proxy } from '../../3-transform/client/utils.js';
Expand Down Expand Up @@ -77,46 +78,6 @@ export function Identifier(node, context) {
if (node.name === '$$restProps') {
context.state.analysis.uses_rest_props = true;
}

if (
binding?.kind === 'normal' &&
((binding.scope === context.state.instance_scope &&
binding.declaration_kind !== 'function') ||
binding.declaration_kind === 'import')
) {
if (
binding.declaration_kind !== 'import' &&
binding.mutated &&
// TODO could be more fine-grained - not every mention in the template implies a state binding
(context.state.reactive_statement || context.state.ast_type === 'template')
) {
binding.kind = 'state';
} else if (
context.state.reactive_statement &&
parent.type === 'AssignmentExpression' &&
parent.left === binding.node
) {
binding.kind = 'derived';
}
} else if (binding?.kind === 'each' && binding.mutated) {
// Ensure that the array is marked as reactive even when only its entries are mutated
let i = context.path.length;
while (i--) {
const ancestor = context.path[i];
if (
ancestor.type === 'EachBlock' &&
context.state.analysis.template.scopes.get(ancestor)?.declarations.get(node.name) ===
binding
) {
for (const binding of ancestor.metadata.references) {
if (binding.kind === 'normal') {
binding.kind = 'state';
}
}
break;
}
}
}
}

if (binding) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,6 @@ export function StyleDirective(node, context) {
let binding = context.state.scope.get(node.name);

if (binding) {
if (!context.state.analysis.runes && binding.mutated) {
binding.kind = 'state';
}

if (binding.kind !== 'normal') {
node.metadata.expression.has_state = true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ export function is_prop_source(binding, state) {
binding.initial ||
// Until legacy mode is gone, we also need to use the prop source when only mutated is true,
// because the parent could be a legacy component which needs coarse-grained reactivity
binding.mutated)
binding.updated)
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ export function EachBlock(node, context) {
const indirect_dependencies = collect_parent_each_blocks(context).flatMap((block) => {
const array = /** @type {Expression} */ (context.visit(block.expression));
const transitive_dependencies = build_transitive_dependencies(
block.metadata.references,
block.metadata.expression.dependencies,
context
);
return [array, ...transitive_dependencies];
Expand All @@ -126,7 +126,7 @@ export function EachBlock(node, context) {
indirect_dependencies.push(collection);

const transitive_dependencies = build_transitive_dependencies(
each_node_meta.references,
each_node_meta.expression.dependencies,
context
);
indirect_dependencies.push(...transitive_dependencies);
Expand Down Expand Up @@ -279,7 +279,7 @@ function collect_parent_each_blocks(context) {
}

/**
* @param {Binding[]} references
* @param {Set<Binding>} references
* @param {ComponentContext} context
*/
function build_transitive_dependencies(references, context) {
Expand Down
Loading
Loading