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

fix: allow ts casts in bindings #10181

Merged
merged 6 commits into from
Jan 19, 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
5 changes: 5 additions & 0 deletions .changeset/selfish-dragons-knock.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"svelte": patch
---

fix: allow ts casts in bindings
8 changes: 5 additions & 3 deletions packages/svelte/src/compiler/phases/2-analyze/validation.js
Original file line number Diff line number Diff line change
Expand Up @@ -338,17 +338,19 @@ export const validation = {
BindDirective(node, context) {
validate_no_const_assignment(node, node.expression, context.state.scope, true);

let left = node.expression;
const assignee = unwrap_ts_expression(node.expression);
let left = assignee;

while (left.type === 'MemberExpression') {
left = /** @type {import('estree').MemberExpression} */ (left.object);
left = unwrap_ts_expression(/** @type {import('estree').MemberExpression} */ (left.object));
}

if (left.type !== 'Identifier') {
error(node, 'invalid-binding-expression');
}

if (
node.expression.type === 'Identifier' &&
assignee.type === 'Identifier' &&
node.name !== 'this' // bind:this also works for regular variables
) {
const binding = context.state.scope.get(left.name);
Expand Down
19 changes: 10 additions & 9 deletions packages/svelte/src/compiler/phases/3-transform/client/utils.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as b from '../../../utils/builders.js';
import { extract_paths, is_simple_expression } from '../../../utils/ast.js';
import { extract_paths, is_simple_expression, unwrap_ts_expression } from '../../../utils/ast.js';
import { error } from '../../../errors.js';
import {
PROPS_IS_LAZY_INITIAL,
Expand Down Expand Up @@ -223,10 +223,11 @@ function is_expression_async(expression) {
export function serialize_set_binding(node, context, fallback, options) {
const { state, visit } = context;

const assignee = unwrap_ts_expression(node.left);
if (
node.left.type === 'ArrayPattern' ||
node.left.type === 'ObjectPattern' ||
node.left.type === 'RestElement'
assignee.type === 'ArrayPattern' ||
assignee.type === 'ObjectPattern' ||
assignee.type === 'RestElement'
) {
// Turn assignment into an IIFE, so that `$.set` calls etc don't produce invalid code
const tmp_id = context.state.scope.generate('tmp');
Expand All @@ -237,7 +238,7 @@ export function serialize_set_binding(node, context, fallback, options) {
/** @type {import('estree').Expression[]} */
const assignments = [];

const paths = extract_paths(node.left);
const paths = extract_paths(assignee);

for (const path of paths) {
const value = path.expression?.(b.id(tmp_id));
Expand Down Expand Up @@ -275,11 +276,11 @@ export function serialize_set_binding(node, context, fallback, options) {
}
}

if (node.left.type !== 'Identifier' && node.left.type !== 'MemberExpression') {
error(node, 'INTERNAL', `Unexpected assignment type ${node.left.type}`);
if (assignee.type !== 'Identifier' && assignee.type !== 'MemberExpression') {
error(node, 'INTERNAL', `Unexpected assignment type ${assignee.type}`);
}

let left = node.left;
let left = assignee;

// Handle class private/public state assignment cases
while (left.type === 'MemberExpression') {
Expand Down Expand Up @@ -342,7 +343,7 @@ export function serialize_set_binding(node, context, fallback, options) {
}
}
// @ts-expect-error
left = left.object;
left = unwrap_ts_expression(left.object);
}

if (left.type !== 'Identifier') {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@ import {
extract_paths,
is_event_attribute,
is_text_attribute,
object
object,
unwrap_ts_expression
} from '../../../../utils/ast.js';
import { binding_properties } from '../../../bindings.js';
import {
Expand Down Expand Up @@ -2579,24 +2580,9 @@ export const template_visitors = {
},
BindDirective(node, context) {
const { state, path, visit } = context;

/** @type {import('estree').Expression[]} */
const properties = [];

let expression = node.expression;
while (expression.type === 'MemberExpression') {
properties.unshift(
expression.computed
? /** @type {import('estree').Expression} */ (expression.property)
: b.literal(/** @type {import('estree').Identifier} */ (expression.property).name)
);
expression = /** @type {import('estree').Identifier | import('estree').MemberExpression} */ (
expression.object
);
}

const getter = b.thunk(/** @type {import('estree').Expression} */ (visit(node.expression)));
const assignment = b.assignment('=', node.expression, b.id('$$value'));
const expression = unwrap_ts_expression(node.expression);
const getter = b.thunk(/** @type {import('estree').Expression} */ (visit(expression)));
const assignment = b.assignment('=', expression, b.id('$$value'));
const setter = b.arrow(
[b.id('$$value')],
serialize_set_binding(
Expand Down Expand Up @@ -2716,7 +2702,7 @@ export const template_visitors = {
setter,
/** @type {import('estree').Expression} */ (
// if expression is not an identifier, we know it can't be a signal
node.expression.type === 'Identifier' ? node.expression : undefined
expression.type === 'Identifier' ? expression : undefined
)
);
break;
Expand Down Expand Up @@ -2765,7 +2751,7 @@ export const template_visitors = {
group_getter = b.thunk(
b.block([
b.stmt(serialize_attribute_value(value, context)[1]),
b.return(/** @type {import('estree').Expression} */ (visit(node.expression)))
b.return(/** @type {import('estree').Expression} */ (visit(expression)))
])
);
}
Expand Down
17 changes: 14 additions & 3 deletions packages/svelte/src/compiler/utils/ast.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { error } from '../errors.js';
import * as b from '../utils/builders.js';

/**
Expand All @@ -7,10 +6,13 @@ import * as b from '../utils/builders.js';
* @returns {import('estree').Identifier | null}
*/
export function object(expression) {
expression = unwrap_ts_expression(expression);

while (expression.type === 'MemberExpression') {
expression = /** @type {import('estree').MemberExpression | import('estree').Identifier} */ (
expression.object
);
expression = unwrap_ts_expression(expression);
}

if (expression.type !== 'Identifier') {
Expand Down Expand Up @@ -270,6 +272,9 @@ function _extract_paths(assignments = [], param, expression, update_expression)
* The Acorn TS plugin defines `foo!` as a `TSNonNullExpression` node, and
* `foo as Bar` as a `TSAsExpression` node. This function unwraps those.
*
* We can't just remove the typescript AST nodes in the parser stage because subsequent
* parsing would fail, since AST start/end nodes would point at the wrong positions.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried removing all AST nodes right after parsing in a post-process step, but it failed because of this - if you have an idea how to solve this, I'm all ears

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's for removing it during parsing right? We could do one pass after the whole file is parsed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mhm maybe. I removed it after an expression/the script tag was parsed, but doing it after the whole thing is parsed could maybe work.. would you mind trying it out?

*
* @template {import('#compiler').SvelteNode | undefined | null} T
* @param {T} node
* @returns {T}
Expand All @@ -279,8 +284,14 @@ export function unwrap_ts_expression(node) {
return node;
}

// @ts-expect-error these types don't exist on the base estree types
if (node.type === 'TSNonNullExpression' || node.type === 'TSAsExpression') {
if (
// @ts-expect-error these types don't exist on the base estree types
node.type === 'TSNonNullExpression' ||
// @ts-expect-error these types don't exist on the base estree types
node.type === 'TSAsExpression' ||
// @ts-expect-error these types don't exist on the base estree types
node.type === 'TSSatisfiesExpression'
) {
// @ts-expect-error
return node.expression;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { test } from '../../test';

export default test({
html: '1 2'
html: '1 2 <div></div> <input type="number"> <input type="number">',
ssrHtml: '1 2 <div></div> <input type="number" value="1"> <input type="number" value="2">'
});
Original file line number Diff line number Diff line change
@@ -1,6 +1,14 @@
<script lang="ts">
let count = $state(1) as number;
let double = $derived(count as number * 2) as number;

let element = null;
let with_state = $state({ foo: 1 });
let without_state = { foo: 2 };
</script>

{count as number} {double as number}

<div bind:this={element as HTMLElement}></div>
<input type="number" bind:value={(with_state as { foo: number }).foo} />
<input type="number" bind:value={(without_state as { foo: number }).foo as number} />
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { test } from '../../test';

export default test({
html: '1 2'
html: '1 2 <input type="number">'
});
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
<script lang="ts">
let count = $state(1)!;
let double = $derived(count! * 2)!;
let binding = $state(null);
</script>

{count!} {double!}

<input type="number" bind:value={binding!} />