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

feat: use typescript-eslint's recommended-requiring-type-checking #1036

Conversation

JoshuaKGoldberg
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg commented Dec 31, 2022

Closes #1035

✅ Checklist

  • I have followed every step in the contributing guide (updated 2022-10-06).
  • The PR title follows the convention we established conventional-commit
  • I performed a functional test on my final commit

Changelog

Augments the internal/CLI and user-generated ESLint configs to extend from plugin:@typescript-eslint/recommended-requiring-type-checking.


Screenshots

...n/a?

💯

@changeset-bot
Copy link

changeset-bot bot commented Dec 31, 2022

🦋 Changeset detected

Latest commit: 61916e5

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

This PR includes changesets to release 1 package
Name Type
create-t3-app Minor

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

@vercel
Copy link

vercel bot commented Dec 31, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
create-t3-app ✅ Ready (Inspect) Visit Preview Jan 7, 2023 at 10:15PM (UTC)

@github-actions github-actions bot added 📌 area: cli Relates to the CLI 📚 documentation Improvements or additions to documentation labels Dec 31, 2022
@JoshuaKGoldberg JoshuaKGoldberg changed the title Typescript eslint recommended requiring type checking feat: use typescript-eslint's recommended-requiring-type-checking Dec 31, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Dec 31, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟢 Performance 100
🟢 Accessibility 100
🟢 Best practices 100
🟠 SEO 86
🟠 PWA 54

Lighthouse ran on https://create-t3-app-git-fork-joshuakgoldberg-typescript-1e820b-t3-oss.vercel.app/

Copy link
Member

@c-ehrlich c-ehrlich left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for putting in all this work! After reading back my comments, some of them don't really relate to the PR itself I guess, but this seems like as good a place as any to bring them up.

I'd basically be fine to approve this as is, but want to leave it open for a few days to allow everyone else to read it and discuss.

@JoshuaKGoldberg
Copy link
Contributor Author

that an type is going to be replaced with an interface

That's from the default rule configurations: specifically how @typescript-eslint/consistent-type-definitions is configured. That rule isn't enabled in the recommended or recommended-requiring-type-checking presets. So this PR shouldn't change that. 🙂

@juliusmarminge
Copy link
Member

juliusmarminge commented Jan 6, 2023

@JoshuaKGoldberg I'm getting tons of yellow squigglies in VSCode but no reported errors in the CLI:

Is this what you meant with the anys here and circular dep?

CleanShot 2023-01-07 at 00 51 20

CleanShot 2023-01-07 at 00 51 27

CleanShot 2023-01-07 at 00 51 32@2x

@juliusmarminge
Copy link
Member

This is a super weird one... says unsafe assignment of type any but it says the variable is a string... 🤯
CleanShot 2023-01-07 at 00 59 45

@JoshuaKGoldberg
Copy link
Contributor Author

@juliusmarminge yes, exactly! What you're seeing is a bug: TypeScript says one thing (e.g. string) in both the tsc CLI and in the IDE, but when typescript-eslint asks it for, the type is reported as any. There's a good chance this will be fixed in TypeScript v5. But I'm not sure.

In the meantime, I'll just disable the rules with a sad comment.

Copy link
Member

@juliusmarminge juliusmarminge left a comment

Choose a reason for hiding this comment

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

Awesome job. Thanks a lot!

devvianto605 pushed a commit to devvianto605/create-devviantex-nextjs-app-deprecated that referenced this pull request Jun 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📌 area: cli Relates to the CLI 📌 area: t3-app Relates to the generated T3 App 📚 documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: use typescript-eslint's recommended-requiring-type-checking
7 participants