Skip to content

fix: allow to pass in TS preference to migration #13929

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.

Sign up for GitHub

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

Merged
dummdidumm merged 2 commits into from
Oct 30, 2024
Merged

fix: allow to pass in TS preference to migration #13929

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
149a557
fix: allow to pass in TS preference to migration
dummdidumm Oct 25, 2024
dd148cd
format
benmccann Oct 25, 2024
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/giant-wombats-allow.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

fix: allow to pass in TS preference to migration
13 changes: 8 additions & 5 deletions packages/svelte/src/compiler/migrate/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,10 @@ class MigrationError extends Error {
* May throw an error if the code is too complex to migrate automatically.
*
* @param {string} source
* @param {{filename?: string}} [options]
* @param {{ filename?: string, use_ts?: boolean }} [options]
* @returns {{ code: string; }}
*/
export function migrate(source, { filename } = {}) {
export function migrate(source, { filename, use_ts } = {}) {
let og_source = source;
try {
has_migration_task = false;
Expand Down Expand Up @@ -115,9 +115,12 @@ export function migrate(source, { filename } = {}) {
derived_components: new Map(),
derived_labeled_statements: new Set(),
has_svelte_self: false,
uses_ts: !!parsed.instance?.attributes.some(
(attr) => attr.name === 'lang' && /** @type {any} */ (attr).value[0].data === 'ts'
)
uses_ts:
// Some people could use jsdoc but have a tsconfig.json, so double-check file for jsdoc indicators
(use_ts && !source.includes('@type {')) ||
Copy link
Contributor

@trueadm trueadm Oct 25, 2024

Choose a reason for hiding this comment

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

I think this needs to be more thorough using a regex. It won't pass if you had:

/**
 * @template T
 * @param {T} thing
 * @returns {T}
 */

For example

Copy link
Member Author

@dummdidumm dummdidumm Oct 25, 2024

Choose a reason for hiding this comment

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

The problem with these is that I have seen these appear above typescript functions in the wild. @type on the other hand is a pretty strong indicator

Copy link
Contributor

@trueadm trueadm Oct 25, 2024

Choose a reason for hiding this comment

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

Makes sense. However, if someone specifies use_ts to be true or false, shouldn't that precedence over a check?

Copy link
Member

@paoloricciuti paoloricciuti Oct 26, 2024

Choose a reason for hiding this comment

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

If they use JSDoc wouldn't they omit the lang="TS"?

Copy link
Member Author

@dummdidumm dummdidumm Oct 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If they use JSDoc wouldn't they omit the lang="TS"?

Yes, which is why there's the outer || in which case we see "this is definitely TS because the lang tag is set". This is only about the ambiguity of people using JSDoc but having a tsconfig.json

Makes sense. However, if someone specifies use_ts to be true or false, shouldn't that precedence over a check?

It will be set to true or false by svelte-migrate in sveltejs/kit#12881 based on the presence (or absence) of tsconfig.json, which as pointed out above does not necessarily mean you're using TS, so the check is still needed

!!parsed.instance?.attributes.some(
(attr) => attr.name === 'lang' && /** @type {any} */ (attr).value[0].data === 'ts'
)
};

if (parsed.module) {
Expand Down
5 changes: 5 additions & 0 deletions packages/svelte/tests/migrate/samples/slot-use_ts/_config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { test } from '../../test';

export default test({
use_ts: true
});
1 change: 1 addition & 0 deletions packages/svelte/tests/migrate/samples/slot-use_ts/input.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<slot />
9 changes: 9 additions & 0 deletions packages/svelte/tests/migrate/samples/slot-use_ts/output.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<script>
interface Props {
children?: import('svelte').Snippet;
}

let { children }: Props = $props();
</script>

{@render children?.()}
4 changes: 3 additions & 1 deletion packages/svelte/tests/migrate/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { suite, type BaseTest } from '../suite.js';

interface ParserTest extends BaseTest {
skip_filename?: boolean;
use_ts?: boolean;
logs?: any[];
errors?: any[];
}
Expand All @@ -32,7 +33,8 @@ const { test, run } = suite<ParserTest>(async (config, cwd) => {
}

const actual = migrate(input, {
filename: config.skip_filename ? undefined : `output.svelte`
filename: config.skip_filename ? undefined : `output.svelte`,
use_ts: config.use_ts
}).code;

if (config.logs) {
Expand Down
3 changes: 2 additions & 1 deletion packages/svelte/types/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1292,8 +1292,9 @@ declare module 'svelte/compiler' {
* May throw an error if the code is too complex to migrate automatically.
*
* */
export function migrate(source: string, { filename }?: {
export function migrate(source: string, { filename, use_ts }?: {
filename?: string;
use_ts?: boolean;
} | undefined): {
code: string;
};
Expand Down