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: ensure migrate keeps inline/trailing comments in $props type definition and produces non-broken code #14143

Merged
merged 7 commits into from
Nov 4, 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/mean-seas-give.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

fix: ensure trailing multiline comments on props produce correct code (#14143#issuecomment-2455702689)
5 changes: 5 additions & 0 deletions .changeset/purple-owls-hug.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

fix: ensure migrate keeps inline/trailing comments in $props type definition
72 changes: 65 additions & 7 deletions packages/svelte/src/compiler/migrate/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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} */`;
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -1497,13 +1497,28 @@ function extract_type_and_comment(declarator, state, path) {
str.update(comment_start, comment_end, '');
}

// 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;
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
Expand All @@ -1526,12 +1541,43 @@ function extract_type_and_comment(declarator, state, path) {
?.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 };
return {
type: match[1],
comment: cleaned_comment,
trailing_comment: cleaned_comment_trailing
};
}
}

Expand All @@ -1540,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 };
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 };
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
Expand Down Expand Up @@ -1779,10 +1833,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,
Expand All @@ -1792,6 +1849,7 @@ function handle_identifier(node, state, path) {
optional: member.optional,
type,
comment,
trailing_comment,
type_only: true
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,20 @@
/**
* 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

/*
* 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
**/
</script>
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,12 @@









/**
* @typedef {Object} Props
* @property {string} comment - My wonderful comment
Expand All @@ -17,6 +23,10 @@
* @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
* @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} */
Expand All @@ -26,6 +36,10 @@
one_line,
no_comment,
type_no_comment,
optional = {stuff: true}
optional = {stuff: true},
inline_commented,
inline_commented_merged,
inline_multiline_leading_comment = 'world',
inline_multiline_trailing_comment = 'world'
} = $props();
</script>
10 changes: 10 additions & 0 deletions packages/svelte/tests/migrate/samples/props-ts/input.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,16 @@
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

/*
* 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
**/
</script>

{readonly}
Expand Down
20 changes: 18 additions & 2 deletions packages/svelte/tests/migrate/samples/props-ts/output.svelte
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
<script lang="ts">





interface Props {
/** some comment */
readonly: number;
Expand All @@ -9,18 +12,31 @@
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
/*
* this is a same-line leading multiline comment
**/
inline_multiline_leading_comment?: string;
inline_multiline_trailing_comment?: string; /*
* this is a same-line trailing multiline comment
**/
}

let {
readonly,
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,
inline_multiline_leading_comment = 'world',
inline_multiline_trailing_comment = 'world'
}: Props = $props();
</script>

{readonly}
{optional}
<input bind:value={binding} />
<input bind:value={bindingOptional} />
<input bind:value={bindingOptional} />