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: search for tsconfig relative to eslintrc #1263

Merged
merged 2 commits into from
Mar 16, 2023

Conversation

mkreuzmayr
Copy link
Contributor

✅ 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

I created a t3-app in an existing monorepo and I expect eslint to use the config file that is in the project and not in the turborepo root. The current .eslintrc.cjs always searches for the tsconfig.json in the root of the project. I resolved the path to do a relative seach, which is what most people expect if there lies a tsconfig.json next to the eslintrc.cjs.

💯

@changeset-bot
Copy link

changeset-bot bot commented Mar 6, 2023

🦋 Changeset detected

Latest commit: 875ae63

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 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

@vercel
Copy link

vercel bot commented Mar 6, 2023

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

Name Status Preview Comments Updated
create-t3-app ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 6, 2023 at 7:31PM (UTC)

@github-actions github-actions bot added 📌 area: cli Relates to the CLI 📌 area: t3-app Relates to the generated T3 App labels Mar 6, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2023

Running Lighthouse audit...

@Zamiell
Copy link
Contributor

Zamiell commented Mar 7, 2023

i was just browsing through the recent PRs, and I'll chime in to confirm that i do this pattern in my monorepos as well.

@juliusmarminge
Copy link
Member

don't y'all put this stuff in the root config? i usually do this: https://github.com/t3-oss/create-t3-turbo/blob/bb46ce25a43fb6379f69988ea1f7e14f6e032ebd/.eslintrc.js#L8-L13

@Zamiell
Copy link
Contributor

Zamiell commented Mar 9, 2023

I don't think using the tsconfigRootDir option obviates the problem. My monorepo has that specified, yet I still have to use path.join.

Furthermore, I don't quite understand your config. Why is the root ESLint extending from each project? That seems backwards. The child ESLint configs should be inheriting from the parent, like how normal TypeScript code would work.

@Zamiell
Copy link
Contributor

Zamiell commented Mar 9, 2023

Also, as a side note, the file should have a cjs extension, not a js extension.

@mkreuzmayr
Copy link
Contributor Author

I agree with @Zamiell

@juliusmarminge Using a single eslint config that spans your whole monorepo is a bad practice, especially if it grows in complexity. If you only have two similar projects in your repo (e.g. all of them are using React) it may be fine.

For monorepos that contain different projects in them (some React, Svelte and NodeJS) where every project needs a dedicated eslint config, your approach is not viable. In addition, it also defeats the whole project structure if configuration files are outside the project scope in which they are used.

@mkreuzmayr
Copy link
Contributor Author

Also, as a side note, the file should have a cjs extension, not a js extension.

This should not be a problem as long as type is not set to module in the package.json.

@Zamiell
Copy link
Contributor

Zamiell commented Mar 9, 2023

For monorepos that contain different projects in them (some React, Svelte and NodeJS) where every project needs a dedicated eslint config, your approach is not viable.

Or just consider the common use-case of a monorepo with a back-end and a front-end, both written in TypeScript. The front-end needs to have a tsconfig that specifies browser-related environment (e.g. DOM types), and the back-end needs to have a tsconfig that specifies node-related environment (e.g. Node types). You need to lint each project separately, so it does not make sense to combine both browser+node node together in the monorepo ESLint config.

@Zamiell
Copy link
Contributor

Zamiell commented Mar 9, 2023

This should not be a problem as long as type is not set to module in the package.json.

It's not a problem insofar that it causes any lint errors, but I think it is best practice in 2023 to name your ESLint config file .eslintrc.cjs regardless of what underlying package.json settings your have, because it makes it very explicit what the format of the file is, and allows you to refactor your package.json settings in the future without introducing any bugs into your codebase.

@juliusmarminge
Copy link
Member

The typescript parser options doesnt need to be separate, so I like just specifying the tsconfig paths in root.

As for the rules etc those can extend some common config package and also add new rules on their own (I think I left this as a todo for our monorepo).

Not quite sure what the benefits are of duplicating the tsconfig path in each config?

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.

Anyway, this way it atleast works in monorepos so that's a big win - even though I wouldn't do this myself, no need to provide monorepo config opinions here 😅

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.

Anyway, this way it atleast works in monorepos so that's a big win - even though I wouldn't do this myself, no need to provide monorepo config opinions here 😅

@juliusmarminge juliusmarminge added this pull request to the merge queue Mar 16, 2023
Merged via the queue into t3-oss:next with commit 8dc6e33 Mar 16, 2023
@Zamiell
Copy link
Contributor

Zamiell commented Mar 16, 2023

The typescript parser options doesnt need to be separate, so I like just specifying the tsconfig paths in root.

They need to be separate, because each package has a separate tsconfig. You wouldn't want to be linting foo with the tsconfig.json compiler settings of bar - that would cause spurious errors in your codebase and other problems. As I explained earlier, the problem becomes more pronounced when you have a node package and a browser package in the same repo.

@juliusmarminge
Copy link
Member

juliusmarminge commented Mar 16, 2023

The typescript parser options doesnt need to be separate, so I like just specifying the tsconfig paths in root.

They need to be separate, because each package has a separate tsconfig. You wouldn't want to be linting foo with the tsconfig.json compiler settings of bar - that would cause spurious errors in your codebase and other problems. As I explained earlier, the problem becomes more pronounced when you have a node package and a browser package in the same repo.

If that's the case then you must have misconfigured your tsconfig.include fields.

Let's say I have:

packageA
  src/
    lots-of-files
  some.config.ts
  tsconfig.json
packageB
  src/
    lots-of-other-files
  maybe-some.config.ts
  tsconfig.json
tsconfig.json
.eslintrc.cjs

Then the packageB files will be parsed by the packageB/tsconfig settings and the same for A. This doesn't change if you specify project as an array in the root .eslintrc.

If you get ts settings from B/tsconfig leaking into pacjageA/src then that's not a caused by a bad lint config - but a bad tsconfig setup

@mkreuzmayr
Copy link
Contributor Author

It isn´t about tsconfig´s here, it´s about eslintrc´s. Your root .eslintrc.cjs defines the linting rules for A and B. Most projects need seperate eslint configs with different rulesets. But nevertheless, if this way works for you, it´s fine, I do not want to force you into doing it differently.

@mkreuzmayr
Copy link
Contributor Author

Thanks for merging. 🚀

devvianto605 pushed a commit to devvianto605/create-devviantex-nextjs-app-deprecated that referenced this pull request Jun 9, 2024
* fix: search for tsconfig relative to eslintrc

* chore: add changeset
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants