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: Add support for TS config files #18134

Draft
wants to merge 55 commits into
base: main
Choose a base branch
from

Conversation

aryaemami59
Copy link

@aryaemami59 aryaemami59 commented Feb 21, 2024

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[X] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

This PR:

  • Adds support for TypeScript config files (eslint.config.ts, eslint.config.mts, eslint.config.cts)
  • Adds jiti as a dependency.
  • Conditionally imports jiti based on whether the config file's extension ends with ts.

I went through a lot of different possible ways of implementing it, and this by far seems to be the simplest way of doing it without changing too much of the internals of the package or adding too many dependencies.

Is there anything you'd like reviewers to focus on?

@eslint-github-bot
Copy link

Hi @aryaemami59!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.

  • The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
  • There should be a space following the initial tag and colon, for example 'feat: Message'.
  • The first letter of the tag should be in lowercase
  • The length of the commit message must be less than or equal to 72

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

Copy link

linux-foundation-easycla bot commented Feb 21, 2024

@github-actions github-actions bot added cli Relates to ESLint's command-line interface core Relates to ESLint's core APIs and features labels Feb 21, 2024
Copy link

netlify bot commented Feb 21, 2024

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit 61abc9b
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/6659df080b73150008824fa2

@aryaemami59 aryaemami59 changed the title Add support for TypeScript config files (eslint.config.ts, eslint.config.mts, eslint.config.cts) feat: Add support for TypeScript config files (eslint.config.ts, eslint.config.mts, eslint.config.cts) Feb 21, 2024
@eslint-github-bot
Copy link

Hi @aryaemami59!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.

  • The length of the commit message must be less than or equal to 72

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

@aryaemami59 aryaemami59 changed the title feat: Add support for TypeScript config files (eslint.config.ts, eslint.config.mts, eslint.config.cts) feat: Add support for TS config files Feb 21, 2024
@eslint-github-bot eslint-github-bot bot added the feature This change adds a new feature to ESLint label Feb 21, 2024
lib/eslint/eslint.js Outdated Show resolved Hide resolved
lib/eslint/eslint.js Outdated Show resolved Hide resolved
lib/eslint/eslint.js Outdated Show resolved Hide resolved
@cany748
Copy link

cany748 commented Feb 29, 2024

Wouldn't it be better to use Jiti for this? I think it's perfect for this, it's used in Nuxt and TailwindCSS to load configuration from Typescript files. It is much smaller and faster than Typescript.

@aladdin-add
Copy link
Member

aladdin-add commented Mar 4, 2024

the first-class ts config support has been discussed in #12078 eslint/rfcs#50, and seems we didn't come to an agreement to accept it.

@aladdin-add
Copy link
Member

it should be possible to use ts config in the current config. The easiest way I can think of is to use --config and --loader, something like:

NODE_OPTIONS=--loader=tsx eslint --config=eslint.config.ts xxx

@aryaemami59
Copy link
Author

@cany748 I had to research it to make sure it doesn't create import side effects. But it seems like you were right, it does what I wanted tsx to do without creating side effects.

@aryaemami59
Copy link
Author

@aladdin-add

it should be possible to use ts config in the current config. The easiest way I can think of is to use --config and --loader, something like:

NODE_OPTIONS=--loader=tsx eslint --config=eslint.config.ts xxx

That doesn't work you'll get:

Error: tsx must be loaded with --import instead of --loader

And if you use

NODE_OPTIONS=--import=tsx eslint --config=eslint.config.ts

you'll get a YAMLException error if you have a type annotation anywhere in the config file. And if you don't have a type annotation anywhere you'll still get this error:

Error: ESLint configuration in --config is invalid:
        - Property "" is the wrong type

@aladdin-add
Copy link
Member

I mean flat config (which is the default in eslint v9). If you are using eslint v8, the default is eslintrc, and you need to specify ESLINT_USE_FLAT_CONFIG=true

@aryaemami59
Copy link
Author

@aladdin-add

I mean flat config (which is the default in eslint v9). If you are using eslint v8, the default is eslintrc, and you need to specify ESLINT_USE_FLAT_CONFIG=true

That still won't work with .mts or .cts extensions, it also doesn't respect the type field in package.json.

Out of curiosity what is your opinion in the current solution this PR provides?

@fasttime
Copy link
Member

Thanks for working on this, @aryaemami59! In order to move forward with this pull request, we will need some unit tests to verify that TypeScript config files are being loaded as expected.

My suggestion would be to start looking at these tests and add similar tests to check the behavior with packages that have config files with .ts, .cts and .mts extension in place of .js, .cjs and .mjs respectively. It would be also useful to have a test that checks loading a config file whose path is specified in overrideConfigFile like this one. When you are done, just mark the PR as ready for review. Feel free to ping me if you need any help.

@fasttime fasttime added accepted There is consensus among the team that this change meets the criteria for inclusion and removed needs design Important details about this change need to be discussed labels May 27, 2024
lib/eslint/eslint.js Outdated Show resolved Hide resolved
@aryaemami59
Copy link
Author

@fasttime

Thanks for working on this, @aryaemami59! In order to move forward with this pull request, we will need some unit tests to verify that TypeScript config files are being loaded as expected.

My suggestion would be to start looking at these tests and add similar tests to check the behavior with packages that have config files with .ts, .cts and .mts extension in place of .js, .cjs and .mjs respectively. It would be also useful to have a test that checks loading a config file whose path is specified in overrideConfigFile like this one. When you are done, just mark the PR as ready for review. Feel free to ping me if you need any help.

Thank you so much, I appreciate it, this was the reason why I haven't written any tests yet, thankfully the problem is resolved thanks to @antfu. I will start writing a bunch of tests now!

aryaemami59 and others added 6 commits May 29, 2024 07:18
Co-authored-by: Anthony Fu <anthonyfu117@hotmail.com>
- This is not only done to reduce some potential redundancy down the line, but to ensure that TS config files are able to handle `type` imports as that is something most people are likely going to do.
Note: `isRunningInBun` and `isRunningInDeno` are functions to make treeshaking for consuming libraries easier.
- This is done for mainly 2 reasons:

  1. We don't know how many runtime environments are going to support loading TS files natively in the future, so this saves us having to check for every single one.
  2. This also ensures that we give the user the option of passing their own TS loader of choice through `NODE_OPTIONS` in CLI for example: `NODE_OPTIONS=--import=tsx/esm`, without ESLint getting in the way and potentially causing conflicts between multiple loaders.
@aryaemami59
Copy link
Author

Not sure which approach is the way to go atm, I would love some feedback on this, i'm a little torn between the first and second approach, though I'm kind of leaning towards the second one.

@fasttime
Copy link
Member

Not sure which approach is the way to go atm, I would love some feedback on this, i'm a little torn between the first and second approach, though I'm kind of leaning towards the second one.

I believe the first approach is more predictable. In the second case, loadTSFile() could fail for any reason (e.g. a runtime error), and it's not trivial to determine if the error occurred because the environment doesn't load TypeScript files natively, or else.

@nzakas
Copy link
Member

nzakas commented May 31, 2024

@aryaemami59 @antfu you're both working on PRs for this feature. It would be helpful if you could collaborate to see which approach would be best.

@aryaemami59
Copy link
Author

@nzakas I'd love nothing more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion cli Relates to ESLint's command-line interface core Relates to ESLint's core APIs and features feature This change adds a new feature to ESLint
Projects
Status: Implementing
Development

Successfully merging this pull request may close these issues.

None yet

7 participants