Skip to content

Feature request: support looking up tsconfig.json relative to linted file #101

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

Closed
Jessidhia opened this issue Jan 21, 2019 · 16 comments · Fixed by #6084
Closed

Feature request: support looking up tsconfig.json relative to linted file #101

Jessidhia opened this issue Jan 21, 2019 · 16 comments · Fixed by #6084
Assignees
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement New feature or request package: parser Issues related to @typescript-eslint/parser package: typescript-estree Issues related to @typescript-eslint/typescript-estree
Milestone

Comments

@Jessidhia
Copy link
Contributor

Jessidhia commented Jan 21, 2019

Currently, the eslint-plugin-tslint throws if any of:

      /**
       * The user needs to have configured "project" in their parserOptions
       * for @typescript-eslint/parser
       */
      if (!parserServices || !parserServices.program) {
        throw new Error(
          `You must provide a value for the "parserOptions.project" property for @typescript-eslint/parser`
        );
      }

or lintFile are missing. If lintFile is missing it'll just lint nothing instead of throwing, unless there are inline configs.


parserOptions.project is completely undocumented in @typescript-eslint/parser, and at any rate that would need to be autodetectable for this use case to work. Currently, services are just silently not generated if it's missing:

  const shouldProvideParserServices =
    shouldGenerateServices && extra.projects && extra.projects.length > 0;

This is despite there being a getASTAndDefaultProject, which looks like it could do the right thing. It's just unreachable code unless you already gave a project to begin with.

On further reading, it looks like the tsconfig.json can be relative, but it's by default calculated relative to process.cwd() and not to the current eslintrc being processed, or an ancestor lookup relative to the current file. The option to change that (tsconfigRootDir) is also undocumented and still can't do what I want.


Secondly, it should be possible to make the tslint.json also be relative to either the current tsconfig.json, the current eslintrc, or to also do a ancestor lookup from the current file.


I think that for most people's use cases the process.cwd() or path.resolve(__dirname, "tslint.json") approach could work, but in my use case I'm trying to make a single shareable eslint config for a monorepo and simply reuse it with "eslintConfig": { "extends": ["@scope/eslint-config"] }. This works for me for everything except these use cases. Note that path.resolve(__dirname, "tslint.json") would still be wrong if a specific project has a different tslint.json than the one the shared eslint-config has.

Even if I leave it to run per project, I still have some specific folders that have a different tsconfig.json nested inside an outer tsconfig.json that belongs to the same project. e.g. a service worker would have worker instead of dom in its lib config. I can still probably deal with this through refactoring, but it sounds like something that could/should be supported; or at least vscode can support it.

@Jessidhia Jessidhia added the triage Waiting for team members to take a look label Jan 21, 2019
@armano2 armano2 added enhancement New feature or request package: parser Issues related to @typescript-eslint/parser package: typescript-estree Issues related to @typescript-eslint/typescript-estree and removed triage Waiting for team members to take a look labels Jan 21, 2019
@j-f1
Copy link
Contributor

j-f1 commented Jan 21, 2019

I think the rationale here was to avoid generating some objects needed to support the type checking if the user doesn’t request them.

@SimenB
Copy link
Contributor

SimenB commented Feb 1, 2019

Got hit by this as well. I'd like to use the new no-unnecessary-type-assertion rule, but is unable to due to our repo being a monorepo, with each package having its own tsconfig. extending either tsconfg.node.json or tsconfig.browser.json in the root of the repo (also using composite projects, fwiw).

I think the rationale here was to avoid generating some objects needed to support the type checking if the user doesn’t request them.

Can I say "please generate the objects needed, I want type info"? Seems like something that should be a boolean turning "look up closest tsconfig.json" on or off

@j-f1
Copy link
Contributor

j-f1 commented Feb 2, 2019

project: true?

@SimenB
Copy link
Contributor

SimenB commented Feb 2, 2019

Yeah, something like that'd be perfect :)

@janbiasi

This comment was marked as spam.

@bradzacher
Copy link
Member

bradzacher commented Jun 6, 2019

We won't ever auto-detect the project without a config item. As soon as we do that, we would force every user to go through a complete typescript compile by just including one of rules.
A lot of users currently avoid rules that require typeinfo because they can cause large perf impacts on some codebases.

However with something like project: true maybe we could use a lookup algorithm instead of requiring a specific config setup.


This can be worked around using tsconfigRootDir:

module.exports = {
    parserOptions: {
        project: './tsconfig.json',
        tsconfigRootDir: __dirname,
    },
}

https://github.com/typescript-eslint/typescript-eslint/tree/master/packages/parser#configuration

If you don't specify tsconfigRootDir, we default to using the CWD because that's how the rest of node works, and how the typescript compiler works (I looked through typescript's code to see what they do).

i.e. if you were to unravel all of our code and typescript's code, you'll see that essentially the algorithm for resolving the provided path is essentially:

const rawConfig = fs.readFileSync(
  path.resolve(
    process.cwd(),
    options.tsconfigRootDir || process.cwd(), // note path.resolve(process.cwd(), process.cwd()) === process.cwd()
    options.project,
  ),
);

@Jessidhia
Copy link
Contributor Author

The idea was not to "autodetect when a rule that needs it is included", but to have an option where which tsconfig.json is used to check the file is not hardcoded in the config, but looked up in relation to the file being linted.

The requirement to be able to specify __dirname also prevents shared configs from being used via json or package.json configs and the individual configs must be written as a commonjs module.

It is a possible workaround, but one that is harder to make use of than an option to enable searching upwards for a tsconfig.json; and I still don't know what happens if different files are linted with different effective tsconfig.json files, as would happen in a monorepo eslint pass.

@bradzacher
Copy link
Member

bradzacher commented Jun 6, 2019

what happens if different files are linted with different effective tsconfig.json files, as would happen in a monorepo eslint pass

If you have a tsconfig.json in your root (which I think you need for vscode to handle it??) then it will work as expected as long as your compiler options are consistent between projects in a monorepo.
This is how we've got it setup in this repo - a root tsconfig which defines the compiler options and then tsconfigs for each project which extend that root config and define the output paths and includes etc.

individual configs must be written as a commonjs module

I didn't think this was a huge problem - from the eyes of your codebase, it's 2 letters in an extension plus a one line change?


Don't get me wrong, I do understand that it's not a perfect solution, but it's a workaround for now.
Happy for someone to PR a change for this though.

@JoshuaKGoldberg JoshuaKGoldberg added the accepting prs Go ahead, send a pull request that resolves this issue label Oct 25, 2021
@bencelang
Copy link

This would be extremely useful in monorepos, where you have some projects with per-project unique rulesets and you want to be able to run eslint from the monorepo root and the projects roots as well. I prefer yaml for configuration and __dirname is not an option in that case

@bradzacher bradzacher changed the title Feature request: support looking up tsconfig.json / tslint.json relative to linted file Feature request: support looking up tsconfig.json relative to linted file Jun 7, 2022
@bradzacher
Copy link
Member

I prefer yaml for configuration

@bencelang I have bad news for you.
YAML configs are going away in a future version of ESLint (as are JSON configs).
https://github.com/eslint/rfcs/tree/main/designs/2019-config-simplification#the-eslintconfigjs-file

I would migrate away from YAML to JS to avoid future pain.

@bencelang
Copy link

@bradzacher Thanks for the heads up! I'll give the JS migration a shot throughout my projects.

individual configs must be written as a commonjs module

However, are ESM config files expected to be supported at some point? One of the reasons why we started using YAML configuration was to help avoid mixed commonjs/esm codebases

@bradzacher
Copy link
Member

That RFC was added in 2019, before ESM was properly supported by node. ESLint has since added support for the .cjs extension for the config file - which you can (and should) use if you've got an ESM project.

The ESLint team is currently discussing the migration to ESM. It's a complicated topic to navigate for such a prominent package.

@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commented Nov 25, 2022

This issue is very old but very juicy - I'd love to be able to shorthand project: true instead of specifying other files. Adding to the v6 milestone. 😄

Edit: this might land in v5 anyway - in which case I'll remove it from the milestone. v6.0.0 is just the latest version I'd be happy with this shipping in.

@JoshuaKGoldberg JoshuaKGoldberg added this to the 6.0.0 milestone Nov 25, 2022
@bradzacher
Copy link
Member

It's worth noting that this would cause some perf regressions because disk lookups are going to be more expensive than a static config. Also depending on project setup you might have some "useless" tsconfigs that we waste time interrogating before finding the correct tsconfig for a file.

In terms of the config style - project: true probably isn't quite good enough for all usecases. For example it's not uncommon to define tsconfig.eslint.json (or other named tsconfigs).
Maybe we'll need to do something like this:

  • project: true - just use the closest tsconfig.json on disk for each linted file.
  • project: { allowedTsconfigNames: FileName[], ignoredTsconfigs: RelativePath[] }
    • allowedTsconfigNames to specify the set of names you want us to search (default ['tsconfig.json']
    • ignoredTsconfigs to ignore specific tsconfigs you know would be useless.

The complicated thing in this algorithm will be determining which tsconfig to use and cache it. It's completely possible for two files in the same folder to be covered by different tsconfigs - so we can't just assume all files in a given folder are covered by the same tsconfig. This means that for every single file we'll need to do a lookup to figure out the tsconfig to use. I believe we can use TS's infra to parse the tsconfig and get the list of files it contains without having to go through the expensive program generation (which should save some time).

For CLI runs we can assume tsconfigs are constant and aggressively cache all this stuff so we only need calculate it once per tsconfig. It should be relatively simple to implement for this usecase.

For persistent IDE runs it's going to be a lot more complicated because we can't attach disk watchers and we can't assume that tsconfigs are static in content or location. I don't know the best way for us to handle this - maybe we need to investigate how vscode does it? I assume they probably have file watchers. We have invalidation logic which handles the "tsconfig was changed" usecase, but I don't know how we can efficiently handle the "tsconfig was added/moved" usecase without watchers. Maybe we'll just need to fuzzily cache for some time period for perf reasons and hope that it doesn't cause too much weirdness? Not sure what the best solution will be!

@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commented Nov 25, 2022

project: true probably isn't quite good enough for all usecases
something like this: ...

Agreed, and that feels like a followup issue to me 😄. Let's get true to "just" work out of the box for users first, and then add in more controls around it?

fuzzily cache for some time period for perf reasons

Yeah, I've been playing with this. I verified locally that createParseSettings is called whenever a file is linted in VS Code, so as long as the "does this tsconfig.json file exist on disk?" cache doesn't last more than a few dozen milliseconds, I feel comfortable going with a straightforward approach like that.

Screenshot of VS Code on createParseSettings source code with the Output channel showing many console logs of the file name

@SimenB
Copy link
Contributor

SimenB commented Feb 13, 2023

Alt Text

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement New feature or request package: parser Issues related to @typescript-eslint/parser package: typescript-estree Issues related to @typescript-eslint/typescript-estree
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants