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.

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
merged 2 commits into from
Oct 30, 2024

Conversation

dummdidumm
Copy link
Member

First half of sveltejs/kit#12880 - the idea is for the migration script to check for a tsconfig.json and then set it to true if one is found

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

Sorry, something went wrong.

Verified

This commit was signed with the committer’s verified signature.
ctavan Christoph Tavan
First half of sveltejs/kit#12880 - the idea is for the migration script to check for a tsconfig.json and then set it to `true` if one is found
Copy link

changeset-bot bot commented Oct 25, 2024

🦋 Changeset detected

Latest commit: dd148cd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

pkg-pr-new bot commented Oct 25, 2024

pnpm add https://pkg.pr.new/svelte@13929

commit: dd148cd

dummdidumm added a commit to sveltejs/kit that referenced this pull request Oct 25, 2024
)
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

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

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

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

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

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

@dummdidumm dummdidumm merged commit cbc2ca3 into main Oct 30, 2024
10 checks passed
@dummdidumm dummdidumm deleted the migration-allow-ts-preference branch October 30, 2024 10:31
@github-actions github-actions bot mentioned this pull request Oct 30, 2024
dummdidumm added a commit to sveltejs/kit that referenced this pull request Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants