From 0a6fa8ab23cd9f410152a5dd8a8df840f526ac25 Mon Sep 17 00:00:00 2001 From: Gonzalo Ruiz Date: Mon, 4 Nov 2024 12:43:35 +0100 Subject: [PATCH 1/7] feat[sv migrate]: keep inline/trailing comments when migrating export let types to type definition --- packages/svelte/src/compiler/migrate/index.js | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/packages/svelte/src/compiler/migrate/index.js b/packages/svelte/src/compiler/migrate/index.js index 3513098a2088..d4d282cf0b3e 100644 --- a/packages/svelte/src/compiler/migrate/index.js +++ b/packages/svelte/src/compiler/migrate/index.js @@ -279,7 +279,7 @@ export function migrate(source, { filename, use_ts } = {}) { type = `interface ${type_name} {${newline_separator}${state.props .map((prop) => { const comment = prop.comment ? `${prop.comment}${newline_separator}` : ''; - return `${comment}${prop.exported}${prop.optional ? '?' : ''}: ${prop.type};`; + return `${comment}${prop.exported}${prop.optional ? '?' : ''}: ${prop.type};${prop.trailing_comment ? ' //' + prop.trailing_comment : ''}`; }) .join(newline_separator)}`; if (analysis.uses_props || analysis.uses_rest_props) { @@ -289,7 +289,7 @@ export function migrate(source, { filename, use_ts } = {}) { } else { type = `/**\n${indent} * @typedef {Object} ${type_name}${state.props .map((prop) => { - return `\n${indent} * @property {${prop.type}} ${prop.optional ? `[${prop.exported}]` : prop.exported}${prop.comment ? ` - ${prop.comment}` : ''}`; + return `\n${indent} * @property {${prop.type}} ${prop.optional ? `[${prop.exported}]` : prop.exported}${prop.comment ? ` - ${prop.comment}` : ''}${prop.trailing_comment ? ` - ${prop.trailing_comment.trim()}` : ''}`; }) .join(``)}\n${indent} */`; } @@ -414,7 +414,7 @@ export function migrate(source, { filename, use_ts } = {}) { * analysis: ComponentAnalysis; * filename?: string; * indent: string; - * props: Array<{ local: string; exported: string; init: string; bindable: boolean; slot_name?: string; optional: boolean; type: string; comment?: string; type_only?: boolean; needs_refine_type?: boolean; }>; + * props: Array<{ local: string; exported: string; init: string; bindable: boolean; slot_name?: string; optional: boolean; type: string; comment?: string; trailing_comment?: string; type_only?: boolean; needs_refine_type?: boolean; }>; * props_insertion_point: number; * has_props_rune: boolean; * has_type_or_fallback: boolean; @@ -1497,13 +1497,15 @@ function extract_type_and_comment(declarator, state, path) { str.update(comment_start, comment_end, ''); } + const trailing_comment = /** @type {Node} */ (parent)?.trailingComments?.at(0)?.value; + if (declarator.id.typeAnnotation) { state.has_type_or_fallback = true; let start = declarator.id.typeAnnotation.start + 1; // skip the colon while (str.original[start] === ' ') { start++; } - return { type: str.original.substring(start, declarator.id.typeAnnotation.end), comment }; + return { type: str.original.substring(start, declarator.id.typeAnnotation.end), comment, trailing_comment }; } let cleaned_comment_arr = comment @@ -1531,7 +1533,7 @@ function extract_type_and_comment(declarator, state, path) { state.has_type_or_fallback = true; const match = /@type {(.+)}/.exec(comment_node.value); if (match) { - return { type: match[1], comment: cleaned_comment }; + return { type: match[1], comment: cleaned_comment, trailing_comment }; } } @@ -1540,11 +1542,11 @@ function extract_type_and_comment(declarator, state, path) { state.has_type_or_fallback = true; // only assume type if it's trivial to infer - else someone would've added a type annotation const type = typeof declarator.init.value; if (type === 'string' || type === 'number' || type === 'boolean') { - return { type, comment: state.uses_ts ? comment : cleaned_comment }; + return { type, comment: state.uses_ts ? comment : cleaned_comment, trailing_comment }; } } - return { type: 'any', comment: state.uses_ts ? comment : cleaned_comment }; + return { type: 'any', comment: state.uses_ts ? comment : cleaned_comment, trailing_comment }; } // Ensure modifiers are applied in the same order as Svelte 4 @@ -1779,10 +1781,13 @@ function handle_identifier(node, state, path) { comment = state.str.original.substring(comment_node.start, comment_node.end); } + const trailing_comment = member.trailingComments?.at(0)?.value; + if (prop) { prop.type = type; prop.optional = member.optional; prop.comment = comment ?? prop.comment; + prop.trailing_comment = trailing_comment ?? prop.trailing_comment; } else { state.props.push({ local: member.key.name, @@ -1792,6 +1797,7 @@ function handle_identifier(node, state, path) { optional: member.optional, type, comment, + trailing_comment, type_only: true }); } From 25edbd38840bdfcd0c495dadd4cb9783ae5796f2 Mon Sep 17 00:00:00 2001 From: Gonzalo Ruiz Date: Mon, 4 Nov 2024 12:57:49 +0100 Subject: [PATCH 2/7] test: add tests for inline comment migration --- .../migrate/samples/jsdoc-with-comments/input.svelte | 9 ++++++++- .../migrate/samples/jsdoc-with-comments/output.svelte | 9 ++++++++- .../svelte/tests/migrate/samples/props-ts/input.svelte | 2 ++ .../svelte/tests/migrate/samples/props-ts/output.svelte | 6 +++++- 4 files changed, 23 insertions(+), 3 deletions(-) diff --git a/packages/svelte/tests/migrate/samples/jsdoc-with-comments/input.svelte b/packages/svelte/tests/migrate/samples/jsdoc-with-comments/input.svelte index 333980513a97..95b998a5b1e5 100644 --- a/packages/svelte/tests/migrate/samples/jsdoc-with-comments/input.svelte +++ b/packages/svelte/tests/migrate/samples/jsdoc-with-comments/input.svelte @@ -24,5 +24,12 @@ /** * This is optional */ - export let optional = {stuff: true}; + export let optional = {stuff: true}; + + export let inline_commented; // this should stay a comment + + /** + * This comment should be merged + */ + export let inline_commented_merged; // with this inline comment \ No newline at end of file diff --git a/packages/svelte/tests/migrate/samples/jsdoc-with-comments/output.svelte b/packages/svelte/tests/migrate/samples/jsdoc-with-comments/output.svelte index 271dd23ffbce..61416e3612dd 100644 --- a/packages/svelte/tests/migrate/samples/jsdoc-with-comments/output.svelte +++ b/packages/svelte/tests/migrate/samples/jsdoc-with-comments/output.svelte @@ -9,6 +9,9 @@ + + + /** * @typedef {Object} Props * @property {string} comment - My wonderful comment @@ -17,6 +20,8 @@ * @property {any} no_comment * @property {boolean} type_no_comment * @property {any} [optional] - This is optional + * @property {any} inline_commented - this should stay a comment + * @property {any} inline_commented_merged - This comment should be merged - with this inline comment */ /** @type {Props} */ @@ -26,6 +31,8 @@ one_line, no_comment, type_no_comment, - optional = {stuff: true} + optional = {stuff: true}, + inline_commented, + inline_commented_merged } = $props(); \ No newline at end of file diff --git a/packages/svelte/tests/migrate/samples/props-ts/input.svelte b/packages/svelte/tests/migrate/samples/props-ts/input.svelte index 34007c08b767..a90da1bee28f 100644 --- a/packages/svelte/tests/migrate/samples/props-ts/input.svelte +++ b/packages/svelte/tests/migrate/samples/props-ts/input.svelte @@ -6,6 +6,8 @@ export let bindingOptional: string | undefined = 'bar'; /** this should stay a comment */ export let no_type_but_comment = 0; + export let type_and_inline_comment:number; // this should also stay a comment + export let no_type_and_inline_comment = 0; // this should stay as well {readonly} diff --git a/packages/svelte/tests/migrate/samples/props-ts/output.svelte b/packages/svelte/tests/migrate/samples/props-ts/output.svelte index 36a0b0f4fb58..a3cb47150170 100644 --- a/packages/svelte/tests/migrate/samples/props-ts/output.svelte +++ b/packages/svelte/tests/migrate/samples/props-ts/output.svelte @@ -9,6 +9,8 @@ bindingOptional?: string | undefined; /** this should stay a comment */ no_type_but_comment?: number; + type_and_inline_comment: number; // this should also stay a comment + no_type_and_inline_comment?: number; // this should stay as well } let { @@ -16,7 +18,9 @@ optional = 'foo', binding = $bindable(), bindingOptional = $bindable('bar'), - no_type_but_comment = 0 + no_type_but_comment = 0, + type_and_inline_comment, + no_type_and_inline_comment = 0 }: Props = $props(); From 1524fac923e5b9e286cb5f40d5081d9a5ac09ef5 Mon Sep 17 00:00:00 2001 From: Gonzalo Ruiz Date: Mon, 4 Nov 2024 17:37:52 +0100 Subject: [PATCH 3/7] chore: add changeset --- .changeset/purple-owls-hug.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/purple-owls-hug.md diff --git a/.changeset/purple-owls-hug.md b/.changeset/purple-owls-hug.md new file mode 100644 index 000000000000..f3dcb0a5e391 --- /dev/null +++ b/.changeset/purple-owls-hug.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: ensure migrate keeps inline/trailing comments in $props type definition From e8b00a2d3e56258e7e780b8ea7f1289c2edff733 Mon Sep 17 00:00:00 2001 From: Gonzalo Ruiz Date: Mon, 4 Nov 2024 22:46:58 +0100 Subject: [PATCH 4/7] fix: migrate trailing multiline comment parsing no longer results in broken code, FIXES PR #14143#issuecomment-2455702689 --- packages/svelte/src/compiler/migrate/index.js | 38 ++++++++++++++++--- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/packages/svelte/src/compiler/migrate/index.js b/packages/svelte/src/compiler/migrate/index.js index d4d282cf0b3e..73dc7bbb199a 100644 --- a/packages/svelte/src/compiler/migrate/index.js +++ b/packages/svelte/src/compiler/migrate/index.js @@ -279,7 +279,7 @@ export function migrate(source, { filename, use_ts } = {}) { type = `interface ${type_name} {${newline_separator}${state.props .map((prop) => { const comment = prop.comment ? `${prop.comment}${newline_separator}` : ''; - return `${comment}${prop.exported}${prop.optional ? '?' : ''}: ${prop.type};${prop.trailing_comment ? ' //' + prop.trailing_comment : ''}`; + return `${comment}${prop.exported}${prop.optional ? '?' : ''}: ${prop.type};${prop.trailing_comment ? ' ' + prop.trailing_comment : ''}`; }) .join(newline_separator)}`; if (analysis.uses_props || analysis.uses_rest_props) { @@ -1497,7 +1497,15 @@ function extract_type_and_comment(declarator, state, path) { str.update(comment_start, comment_end, ''); } - const trailing_comment = /** @type {Node} */ (parent)?.trailingComments?.at(0)?.value; + // Find trailing comments + const trailing_comment_node = /** @type {Node} */ (parent)?.trailingComments?.at(0); + const trailing_comment_start = /** @type {any} */ (trailing_comment_node)?.start; + const trailing_comment_end = /** @type {any} */ (trailing_comment_node)?.end; + let trailing_comment = trailing_comment_node && str.original.substring(trailing_comment_start, trailing_comment_end); + + if (trailing_comment_node) { + str.update(trailing_comment_start, trailing_comment_end, ''); + } if (declarator.id.typeAnnotation) { state.has_type_or_fallback = true; @@ -1527,13 +1535,33 @@ function extract_type_and_comment(declarator, state, path) { let cleaned_comment = cleaned_comment_arr ?.slice(0, first_at_comment !== -1 ? first_at_comment : cleaned_comment_arr.length) .join('\n'); + + let cleaned_comment_arr_trailing = trailing_comment + ?.split('\n') + .map((line) => + line + .trim() + // replace `// ` for one liners + .replace(/^\/\/\s*/g, '') + // replace `\**` for the initial JSDoc + .replace(/^\/\*\*?\s*/g, '') + // migrate `*/` for the end of JSDoc + .replace(/\s*\*\/$/g, '') + // remove any initial `* ` to clean the comment + .replace(/^\*\s*/g, '') + ) + .filter(Boolean); + const first_at_comment_trailing = cleaned_comment_arr_trailing?.findIndex((line) => line.startsWith('@')); + let cleaned_comment_trailing = cleaned_comment_arr_trailing + ?.slice(0, first_at_comment_trailing !== -1 ? first_at_comment_trailing : cleaned_comment_arr_trailing.length) + .join('\n'); // try to find a comment with a type annotation, hinting at jsdoc if (parent?.type === 'ExportNamedDeclaration' && comment_node) { state.has_type_or_fallback = true; const match = /@type {(.+)}/.exec(comment_node.value); if (match) { - return { type: match[1], comment: cleaned_comment, trailing_comment }; + return { type: match[1], comment: cleaned_comment, trailing_comment: cleaned_comment_trailing }; } } @@ -1542,11 +1570,11 @@ function extract_type_and_comment(declarator, state, path) { state.has_type_or_fallback = true; // only assume type if it's trivial to infer - else someone would've added a type annotation const type = typeof declarator.init.value; if (type === 'string' || type === 'number' || type === 'boolean') { - return { type, comment: state.uses_ts ? comment : cleaned_comment, trailing_comment }; + return { type, comment: state.uses_ts ? comment : cleaned_comment, trailing_comment: state.uses_ts ? trailing_comment : cleaned_comment_trailing }; } } - return { type: 'any', comment: state.uses_ts ? comment : cleaned_comment, trailing_comment }; + return { type: 'any', comment: state.uses_ts ? comment : cleaned_comment, trailing_comment: state.uses_ts ? trailing_comment : cleaned_comment_trailing }; } // Ensure modifiers are applied in the same order as Svelte 4 From 308c8bf6b76e9fc4d4b696e9d63f2346f1743607 Mon Sep 17 00:00:00 2001 From: Gonzalo Ruiz Date: Mon, 4 Nov 2024 22:53:35 +0100 Subject: [PATCH 5/7] test: add migrate test with same-line trailing multiline comments and same-line leading multiline comments --- .../samples/jsdoc-with-comments/input.svelte | 8 ++++++++ .../samples/jsdoc-with-comments/output.svelte | 9 ++++++++- .../tests/migrate/samples/props-ts/input.svelte | 8 ++++++++ .../tests/migrate/samples/props-ts/output.svelte | 16 ++++++++++++++-- 4 files changed, 38 insertions(+), 3 deletions(-) diff --git a/packages/svelte/tests/migrate/samples/jsdoc-with-comments/input.svelte b/packages/svelte/tests/migrate/samples/jsdoc-with-comments/input.svelte index 95b998a5b1e5..f2efb1db804c 100644 --- a/packages/svelte/tests/migrate/samples/jsdoc-with-comments/input.svelte +++ b/packages/svelte/tests/migrate/samples/jsdoc-with-comments/input.svelte @@ -32,4 +32,12 @@ * This comment should be merged */ export let inline_commented_merged; // with this inline comment + + /* + * this is a same-line leading multiline comment + **/ export let inline_multiline_leading_comment = 'world'; + + export let inline_multiline_trailing_comment = 'world'; /* + * this is a same-line trailing multiline comment + **/ \ No newline at end of file diff --git a/packages/svelte/tests/migrate/samples/jsdoc-with-comments/output.svelte b/packages/svelte/tests/migrate/samples/jsdoc-with-comments/output.svelte index 61416e3612dd..19fbe38b5093 100644 --- a/packages/svelte/tests/migrate/samples/jsdoc-with-comments/output.svelte +++ b/packages/svelte/tests/migrate/samples/jsdoc-with-comments/output.svelte @@ -12,6 +12,9 @@ + + + /** * @typedef {Object} Props * @property {string} comment - My wonderful comment @@ -22,6 +25,8 @@ * @property {any} [optional] - This is optional * @property {any} inline_commented - this should stay a comment * @property {any} inline_commented_merged - This comment should be merged - with this inline comment + * @property {string} [inline_multiline_leading_comment] - this is a same-line leading multiline comment + * @property {string} [inline_multiline_trailing_comment] - this is a same-line trailing multiline comment */ /** @type {Props} */ @@ -33,6 +38,8 @@ type_no_comment, optional = {stuff: true}, inline_commented, - inline_commented_merged + inline_commented_merged, + inline_multiline_leading_comment = 'world', + inline_multiline_trailing_comment = 'world' } = $props(); \ No newline at end of file diff --git a/packages/svelte/tests/migrate/samples/props-ts/input.svelte b/packages/svelte/tests/migrate/samples/props-ts/input.svelte index a90da1bee28f..9378c54ecfc0 100644 --- a/packages/svelte/tests/migrate/samples/props-ts/input.svelte +++ b/packages/svelte/tests/migrate/samples/props-ts/input.svelte @@ -8,6 +8,14 @@ export let no_type_but_comment = 0; export let type_and_inline_comment:number; // this should also stay a comment export let no_type_and_inline_comment = 0; // this should stay as well + + /* + * this is a same-line leading multiline comment + **/ export let inline_multiline_leading_comment = 'world'; + + export let inline_multiline_trailing_comment = 'world'; /* + * this is a same-line trailing multiline comment + **/ {readonly} diff --git a/packages/svelte/tests/migrate/samples/props-ts/output.svelte b/packages/svelte/tests/migrate/samples/props-ts/output.svelte index a3cb47150170..e9deea13846d 100644 --- a/packages/svelte/tests/migrate/samples/props-ts/output.svelte +++ b/packages/svelte/tests/migrate/samples/props-ts/output.svelte @@ -1,6 +1,9 @@ {readonly} {optional} - \ No newline at end of file + From d8f4cddb013d0f88609affa285cd92f660f5a68b Mon Sep 17 00:00:00 2001 From: Gonzalo Ruiz Date: Mon, 4 Nov 2024 22:57:26 +0100 Subject: [PATCH 6/7] chore: add changeset --- .changeset/mean-seas-give.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/mean-seas-give.md diff --git a/.changeset/mean-seas-give.md b/.changeset/mean-seas-give.md new file mode 100644 index 000000000000..0c16ac6d5514 --- /dev/null +++ b/.changeset/mean-seas-give.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: ensure trailing multiline comments on props produce correct code (#14143#issuecomment-2455702689) From e00d318d5e4a0d8c116a01323d86cff5688cedc6 Mon Sep 17 00:00:00 2001 From: paoloricciuti Date: Mon, 4 Nov 2024 23:28:35 +0100 Subject: [PATCH 7/7] fix: lint --- packages/svelte/src/compiler/migrate/index.js | 40 +++++++++++++++---- 1 file changed, 32 insertions(+), 8 deletions(-) diff --git a/packages/svelte/src/compiler/migrate/index.js b/packages/svelte/src/compiler/migrate/index.js index 73dc7bbb199a..8e8670c8243e 100644 --- a/packages/svelte/src/compiler/migrate/index.js +++ b/packages/svelte/src/compiler/migrate/index.js @@ -1501,7 +1501,8 @@ function extract_type_and_comment(declarator, state, path) { const trailing_comment_node = /** @type {Node} */ (parent)?.trailingComments?.at(0); const trailing_comment_start = /** @type {any} */ (trailing_comment_node)?.start; const trailing_comment_end = /** @type {any} */ (trailing_comment_node)?.end; - let trailing_comment = trailing_comment_node && str.original.substring(trailing_comment_start, trailing_comment_end); + let trailing_comment = + trailing_comment_node && str.original.substring(trailing_comment_start, trailing_comment_end); if (trailing_comment_node) { str.update(trailing_comment_start, trailing_comment_end, ''); @@ -1513,7 +1514,11 @@ function extract_type_and_comment(declarator, state, path) { while (str.original[start] === ' ') { start++; } - return { type: str.original.substring(start, declarator.id.typeAnnotation.end), comment, trailing_comment }; + return { + type: str.original.substring(start, declarator.id.typeAnnotation.end), + comment, + trailing_comment + }; } let cleaned_comment_arr = comment @@ -1535,7 +1540,7 @@ function extract_type_and_comment(declarator, state, path) { let cleaned_comment = cleaned_comment_arr ?.slice(0, first_at_comment !== -1 ? first_at_comment : cleaned_comment_arr.length) .join('\n'); - + let cleaned_comment_arr_trailing = trailing_comment ?.split('\n') .map((line) => @@ -1551,9 +1556,16 @@ function extract_type_and_comment(declarator, state, path) { .replace(/^\*\s*/g, '') ) .filter(Boolean); - const first_at_comment_trailing = cleaned_comment_arr_trailing?.findIndex((line) => line.startsWith('@')); + const first_at_comment_trailing = cleaned_comment_arr_trailing?.findIndex((line) => + line.startsWith('@') + ); let cleaned_comment_trailing = cleaned_comment_arr_trailing - ?.slice(0, first_at_comment_trailing !== -1 ? first_at_comment_trailing : cleaned_comment_arr_trailing.length) + ?.slice( + 0, + first_at_comment_trailing !== -1 + ? first_at_comment_trailing + : cleaned_comment_arr_trailing.length + ) .join('\n'); // try to find a comment with a type annotation, hinting at jsdoc @@ -1561,7 +1573,11 @@ function extract_type_and_comment(declarator, state, path) { state.has_type_or_fallback = true; const match = /@type {(.+)}/.exec(comment_node.value); if (match) { - return { type: match[1], comment: cleaned_comment, trailing_comment: cleaned_comment_trailing }; + return { + type: match[1], + comment: cleaned_comment, + trailing_comment: cleaned_comment_trailing + }; } } @@ -1570,11 +1586,19 @@ function extract_type_and_comment(declarator, state, path) { state.has_type_or_fallback = true; // only assume type if it's trivial to infer - else someone would've added a type annotation const type = typeof declarator.init.value; if (type === 'string' || type === 'number' || type === 'boolean') { - return { type, comment: state.uses_ts ? comment : cleaned_comment, trailing_comment: state.uses_ts ? trailing_comment : cleaned_comment_trailing }; + return { + type, + comment: state.uses_ts ? comment : cleaned_comment, + trailing_comment: state.uses_ts ? trailing_comment : cleaned_comment_trailing + }; } } - return { type: 'any', comment: state.uses_ts ? comment : cleaned_comment, trailing_comment: state.uses_ts ? trailing_comment : cleaned_comment_trailing }; + return { + type: 'any', + comment: state.uses_ts ? comment : cleaned_comment, + trailing_comment: state.uses_ts ? trailing_comment : cleaned_comment_trailing + }; } // Ensure modifiers are applied in the same order as Svelte 4