From caf2675415dfb99bdee15c966d5c987387b92adc Mon Sep 17 00:00:00 2001 From: paoloricciuti Date: Mon, 17 Jun 2024 21:36:49 +0200 Subject: [PATCH 1/8] fix: allow multiple optional parameters with defaults --- .changeset/ten-geese-share.md | 5 +++++ packages/svelte/src/compiler/phases/1-parse/state/tag.js | 9 ++++++++- .../snippet-optional-arguments-defaults/_config.js | 7 +++++++ .../snippet-optional-arguments-defaults/main.svelte | 5 +++++ 4 files changed, 25 insertions(+), 1 deletion(-) create mode 100644 .changeset/ten-geese-share.md create mode 100644 packages/svelte/tests/runtime-runes/samples/snippet-optional-arguments-defaults/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/snippet-optional-arguments-defaults/main.svelte diff --git a/.changeset/ten-geese-share.md b/.changeset/ten-geese-share.md new file mode 100644 index 000000000000..118c10b5dd94 --- /dev/null +++ b/.changeset/ten-geese-share.md @@ -0,0 +1,5 @@ +--- +"svelte": patch +--- + +fix: allow multiple optional parameters with defaults diff --git a/packages/svelte/src/compiler/phases/1-parse/state/tag.js b/packages/svelte/src/compiler/phases/1-parse/state/tag.js index e113469421ab..a7bf23083166 100644 --- a/packages/svelte/src/compiler/phases/1-parse/state/tag.js +++ b/packages/svelte/src/compiler/phases/1-parse/state/tag.js @@ -281,7 +281,14 @@ function open(parser) { parser.allow_whitespace(); if (parser.eat('=')) { parser.allow_whitespace(); - const right = read_expression(parser); + let right = read_expression(parser); + // multiple assignment expression will be confused by the parser for a sequence expression + // so if the returned value is a sequence expression we use the first expression as the + // right and we reset the parser.index + if (right.type === 'SequenceExpression') { + right = right.expressions[0]; + parser.index = right.end ?? parser.index; + } pattern = { type: 'AssignmentPattern', left: pattern, diff --git a/packages/svelte/tests/runtime-runes/samples/snippet-optional-arguments-defaults/_config.js b/packages/svelte/tests/runtime-runes/samples/snippet-optional-arguments-defaults/_config.js new file mode 100644 index 000000000000..181647cdc7d4 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/snippet-optional-arguments-defaults/_config.js @@ -0,0 +1,7 @@ +import { flushSync } from 'svelte'; +import { test } from '../../test'; + +export default test({ + html: '012', + ssrHtml: '012' +}); diff --git a/packages/svelte/tests/runtime-runes/samples/snippet-optional-arguments-defaults/main.svelte b/packages/svelte/tests/runtime-runes/samples/snippet-optional-arguments-defaults/main.svelte new file mode 100644 index 000000000000..bac8f0eccc1e --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/snippet-optional-arguments-defaults/main.svelte @@ -0,0 +1,5 @@ +{#snippet snip(a, b = 1, c=2)} +{a}{b}{c} +{/snippet} + +{@render snip(0)} From 30d944f1ba7e172ba741b16d1d062eb7e0b7ed59 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Mon, 17 Jun 2024 21:10:05 -0400 Subject: [PATCH 2/8] Apply suggestions from code review --- .../samples/snippet-optional-arguments-defaults/_config.js | 3 +-- .../samples/snippet-optional-arguments-defaults/main.svelte | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/svelte/tests/runtime-runes/samples/snippet-optional-arguments-defaults/_config.js b/packages/svelte/tests/runtime-runes/samples/snippet-optional-arguments-defaults/_config.js index 181647cdc7d4..922aa0b88b6d 100644 --- a/packages/svelte/tests/runtime-runes/samples/snippet-optional-arguments-defaults/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/snippet-optional-arguments-defaults/_config.js @@ -2,6 +2,5 @@ import { flushSync } from 'svelte'; import { test } from '../../test'; export default test({ - html: '012', - ssrHtml: '012' + html: '013' }); diff --git a/packages/svelte/tests/runtime-runes/samples/snippet-optional-arguments-defaults/main.svelte b/packages/svelte/tests/runtime-runes/samples/snippet-optional-arguments-defaults/main.svelte index bac8f0eccc1e..40cfe5855f31 100644 --- a/packages/svelte/tests/runtime-runes/samples/snippet-optional-arguments-defaults/main.svelte +++ b/packages/svelte/tests/runtime-runes/samples/snippet-optional-arguments-defaults/main.svelte @@ -1,5 +1,5 @@ -{#snippet snip(a, b = 1, c=2)} -{a}{b}{c} +{#snippet snip(a, b = 1, c = (2, 3))} + {a}{b}{c} {/snippet} {@render snip(0)} From 4b740ae649dc95372fe5efcb192b0135978f4ae4 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Mon, 17 Jun 2024 21:23:21 -0400 Subject: [PATCH 3/8] partial fix --- .../svelte/src/compiler/phases/1-parse/state/tag.js | 12 +++++++++--- .../snippet-optional-arguments-defaults/_config.js | 1 - 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/packages/svelte/src/compiler/phases/1-parse/state/tag.js b/packages/svelte/src/compiler/phases/1-parse/state/tag.js index a7bf23083166..b8c94e805912 100644 --- a/packages/svelte/src/compiler/phases/1-parse/state/tag.js +++ b/packages/svelte/src/compiler/phases/1-parse/state/tag.js @@ -281,18 +281,24 @@ function open(parser) { parser.allow_whitespace(); if (parser.eat('=')) { parser.allow_whitespace(); + + let index = parser.index; let right = read_expression(parser); + + parser.allow_whitespace(); + // multiple assignment expression will be confused by the parser for a sequence expression // so if the returned value is a sequence expression we use the first expression as the // right and we reset the parser.index - if (right.type === 'SequenceExpression') { + if (right.type === 'SequenceExpression' && parser.template[index] !== '(') { right = right.expressions[0]; - parser.index = right.end ?? parser.index; + parser.index = /** @type {number} */ (right.end); } + pattern = { type: 'AssignmentPattern', left: pattern, - right: right, + right, start: pattern.start, end: right.end }; diff --git a/packages/svelte/tests/runtime-runes/samples/snippet-optional-arguments-defaults/_config.js b/packages/svelte/tests/runtime-runes/samples/snippet-optional-arguments-defaults/_config.js index 922aa0b88b6d..80addacd1aed 100644 --- a/packages/svelte/tests/runtime-runes/samples/snippet-optional-arguments-defaults/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/snippet-optional-arguments-defaults/_config.js @@ -1,4 +1,3 @@ -import { flushSync } from 'svelte'; import { test } from '../../test'; export default test({ From 4d931abd9e02d5e458de56c5ba7564b05ec51836 Mon Sep 17 00:00:00 2001 From: paoloricciuti Date: Fri, 21 Jun 2024 19:28:13 +0200 Subject: [PATCH 4/8] feat: parse as a whole function --- .../src/compiler/phases/1-parse/state/tag.js | 88 +++++++------------ 1 file changed, 31 insertions(+), 57 deletions(-) diff --git a/packages/svelte/src/compiler/phases/1-parse/state/tag.js b/packages/svelte/src/compiler/phases/1-parse/state/tag.js index b8c94e805912..961831364a8f 100644 --- a/packages/svelte/src/compiler/phases/1-parse/state/tag.js +++ b/packages/svelte/src/compiler/phases/1-parse/state/tag.js @@ -3,6 +3,7 @@ import read_expression from '../read/expression.js'; import * as e from '../../../errors.js'; import { create_fragment } from '../utils/create.js'; import { walk } from 'zimmerframe'; +import { parse_expression_at } from '../acorn.js'; const regex_whitespace_with_closing_curly_brace = /^\s*}/; @@ -270,68 +271,41 @@ function open(parser) { parser.eat('(', true); - parser.allow_whitespace(); - - /** @type {import('estree').Pattern[]} */ - const parameters = []; - - while (!parser.match(')')) { - let pattern = read_pattern(parser, true); - - parser.allow_whitespace(); - if (parser.eat('=')) { - parser.allow_whitespace(); - - let index = parser.index; - let right = read_expression(parser); - - parser.allow_whitespace(); - - // multiple assignment expression will be confused by the parser for a sequence expression - // so if the returned value is a sequence expression we use the first expression as the - // right and we reset the parser.index - if (right.type === 'SequenceExpression' && parser.template[index] !== '(') { - right = right.expressions[0]; - parser.index = /** @type {number} */ (right.end); - } - - pattern = { - type: 'AssignmentPattern', - left: pattern, - right, - start: pattern.start, - end: right.end - }; - } - - parameters.push(pattern); + let parentheses = 1; + let params = ''; - if (!parser.eat(',')) break; - parser.allow_whitespace(); + while (!parser.match(')') || parentheses !== 1) { + if (parser.match('(')) parentheses++; + if (parser.match(')')) parentheses--; + params += parser.read(/^./); } - parser.eat(')', true); + const function_expression = parse_expression_at(`function ${name}(${params}){}`, parser.ts, 0); - parser.allow_whitespace(); - parser.eat('}', true); + parser.index += 2; - /** @type {ReturnType>} */ - const block = parser.append({ - type: 'SnippetBlock', - start, - end: -1, - expression: { - type: 'Identifier', - start: name_start, - end: name_end, - name - }, - parameters, - body: create_fragment() - }); - - parser.stack.push(block); - parser.fragments.push(block.body); + if (function_expression.type === 'FunctionExpression') { + /** @type {ReturnType>} */ + const block = parser.append({ + type: 'SnippetBlock', + start, + end: -1, + expression: { + type: 'Identifier', + start: name_start, + end: name_end, + name + }, + parameters: function_expression.params.map((param) => { + param.start += start + 1; + param.end += start + 1; + return param; + }), + body: create_fragment() + }); + parser.stack.push(block); + parser.fragments.push(block.body); + } return; } From c3535d2d40dccfebbd02cc8b90807fe7b7689984 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Fri, 21 Jun 2024 14:08:12 -0400 Subject: [PATCH 5/8] couple of fixes --- .../src/compiler/phases/1-parse/state/tag.js | 48 ++++++++++--------- .../samples/snippets/output.json | 15 ++++-- 2 files changed, 37 insertions(+), 26 deletions(-) diff --git a/packages/svelte/src/compiler/phases/1-parse/state/tag.js b/packages/svelte/src/compiler/phases/1-parse/state/tag.js index 961831364a8f..85fde1d4869c 100644 --- a/packages/svelte/src/compiler/phases/1-parse/state/tag.js +++ b/packages/svelte/src/compiler/phases/1-parse/state/tag.js @@ -269,6 +269,8 @@ function open(parser) { e.expected_identifier(parser.index); } + let slice_end = parser.index; + parser.eat('(', true); let parentheses = 1; @@ -280,32 +282,32 @@ function open(parser) { params += parser.read(/^./); } - const function_expression = parse_expression_at(`function ${name}(${params}){}`, parser.ts, 0); + let function_expression = /** @type {import('estree').ArrowFunctionExpression} */ ( + parse_expression_at( + parser.template.slice(0, slice_end).replace(/\S/g, ' ') + `(${params}) => {}`, + parser.ts, + 0 + ) + ); parser.index += 2; - if (function_expression.type === 'FunctionExpression') { - /** @type {ReturnType>} */ - const block = parser.append({ - type: 'SnippetBlock', - start, - end: -1, - expression: { - type: 'Identifier', - start: name_start, - end: name_end, - name - }, - parameters: function_expression.params.map((param) => { - param.start += start + 1; - param.end += start + 1; - return param; - }), - body: create_fragment() - }); - parser.stack.push(block); - parser.fragments.push(block.body); - } + /** @type {ReturnType>} */ + const block = parser.append({ + type: 'SnippetBlock', + start, + end: -1, + expression: { + type: 'Identifier', + start: name_start, + end: name_end, + name + }, + parameters: function_expression.params, + body: create_fragment() + }); + parser.stack.push(block); + parser.fragments.push(block.body); return; } diff --git a/packages/svelte/tests/parser-modern/samples/snippets/output.json b/packages/svelte/tests/parser-modern/samples/snippets/output.json index ac9aa580678a..16a60cde557d 100644 --- a/packages/svelte/tests/parser-modern/samples/snippets/output.json +++ b/packages/svelte/tests/parser-modern/samples/snippets/output.json @@ -32,8 +32,7 @@ "loc": { "start": { "line": 3, - "column": 14, - "character": 43 + "column": 14 }, "end": { "line": 3, @@ -41,11 +40,21 @@ "character": 54 } }, - "end": 54, + "end": 46, "typeAnnotation": { "type": "TSTypeAnnotation", "start": 46, "end": 54, + "loc": { + "start": { + "line": 3, + "column": 17 + }, + "end": { + "line": 3, + "column": 25 + } + }, "typeAnnotation": { "type": "TSStringKeyword", "start": 48, From a92bc2d0ebed8dcf0f459b92445c6705eae96be6 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Fri, 21 Jun 2024 16:26:08 -0400 Subject: [PATCH 6/8] work around acorn-typescript quirks --- packages/svelte/src/compiler/phases/1-parse/acorn.js | 11 +++++++++++ .../tests/parser-modern/samples/snippets/output.json | 3 +-- .../samples/typescript-in-event-handler/output.json | 5 ++++- 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/packages/svelte/src/compiler/phases/1-parse/acorn.js b/packages/svelte/src/compiler/phases/1-parse/acorn.js index 5749cb0d6f75..0da21fe3ce92 100644 --- a/packages/svelte/src/compiler/phases/1-parse/acorn.js +++ b/packages/svelte/src/compiler/phases/1-parse/acorn.js @@ -1,6 +1,7 @@ import * as acorn from 'acorn'; import { walk } from 'zimmerframe'; import { tsPlugin } from 'acorn-typescript'; +import { locator } from '../../state.js'; const ParserWithTS = acorn.Parser.extend(tsPlugin({ allowSatisfies: true })); @@ -127,6 +128,16 @@ function amend(source, node) { // @ts-expect-error delete node.loc.end.index; + if (typeof node.loc?.end === 'number') { + const loc = locator(node.loc.end); + if (loc) { + node.loc.end = { + line: loc.line, + column: loc.column + }; + } + } + if (/** @type {any} */ (node).typeAnnotation && node.end === undefined) { // i think there might be a bug in acorn-typescript that prevents // `end` from being assigned when there's a type annotation diff --git a/packages/svelte/tests/parser-modern/samples/snippets/output.json b/packages/svelte/tests/parser-modern/samples/snippets/output.json index 16a60cde557d..67d30db1b0b1 100644 --- a/packages/svelte/tests/parser-modern/samples/snippets/output.json +++ b/packages/svelte/tests/parser-modern/samples/snippets/output.json @@ -36,8 +36,7 @@ }, "end": { "line": 3, - "column": 25, - "character": 54 + "column": 25 } }, "end": 46, diff --git a/packages/svelte/tests/parser-modern/samples/typescript-in-event-handler/output.json b/packages/svelte/tests/parser-modern/samples/typescript-in-event-handler/output.json index 6f219c7311f2..c3442092a8af 100644 --- a/packages/svelte/tests/parser-modern/samples/typescript-in-event-handler/output.json +++ b/packages/svelte/tests/parser-modern/samples/typescript-in-event-handler/output.json @@ -54,7 +54,10 @@ "line": 6, "column": 12 }, - "end": 87 + "end": { + "line": 6, + "column": 25 + } }, "name": "e", "typeAnnotation": { From 752d3f9ca5bee968ebc677ed35ff162d21d69643 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Fri, 21 Jun 2024 16:29:08 -0400 Subject: [PATCH 7/8] add the harder test --- .../snippet-optional-arguments-defaults/_config.js | 2 +- .../snippet-optional-arguments-defaults/main.svelte | 10 +++++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/packages/svelte/tests/runtime-runes/samples/snippet-optional-arguments-defaults/_config.js b/packages/svelte/tests/runtime-runes/samples/snippet-optional-arguments-defaults/_config.js index 80addacd1aed..e651cb2eb6a2 100644 --- a/packages/svelte/tests/runtime-runes/samples/snippet-optional-arguments-defaults/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/snippet-optional-arguments-defaults/_config.js @@ -1,5 +1,5 @@ import { test } from '../../test'; export default test({ - html: '013' + html: '013/023' }); diff --git a/packages/svelte/tests/runtime-runes/samples/snippet-optional-arguments-defaults/main.svelte b/packages/svelte/tests/runtime-runes/samples/snippet-optional-arguments-defaults/main.svelte index 40cfe5855f31..235381d57c1c 100644 --- a/packages/svelte/tests/runtime-runes/samples/snippet-optional-arguments-defaults/main.svelte +++ b/packages/svelte/tests/runtime-runes/samples/snippet-optional-arguments-defaults/main.svelte @@ -1,5 +1,9 @@ -{#snippet snip(a, b = 1, c = (2, 3))} - {a}{b}{c} +{#snippet one(a, b = 1, c = (2, 3))} + {a}{b}{c} {/snippet} -{@render snip(0)} +{#snippet two(a, b = (1, 2), c = 3)} + {a}{b}{c} +{/snippet} + +{@render one(0)}/{@render two(0)} From f9527080a9352d4ac1101b283eebdfe3aab93f69 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Fri, 21 Jun 2024 16:33:48 -0400 Subject: [PATCH 8/8] Update .changeset/ten-geese-share.md --- .changeset/ten-geese-share.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/ten-geese-share.md b/.changeset/ten-geese-share.md index 118c10b5dd94..4b924c9958e7 100644 --- a/.changeset/ten-geese-share.md +++ b/.changeset/ten-geese-share.md @@ -2,4 +2,4 @@ "svelte": patch --- -fix: allow multiple optional parameters with defaults +fix: allow multiple optional parameters with defaults in snippets