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 TypeScript config files #117

Merged
merged 23 commits into from
May 24, 2024

Conversation

aryaemami59
Copy link
Contributor

Summary

Add support for TypeScript config files (eslint.config.ts, eslint.config.mts, eslint.config.cts).

Related Issues

This PR is related to this RFC.

@aryaemami59 aryaemami59 marked this pull request as ready for review March 9, 2024 18:15
@bradzacher
Copy link

Worth linking the prior art in this area: #50

@kecrily
Copy link
Member

kecrily commented Mar 10, 2024

I think it makes sense and is low cost to support this in a runtime that supports ts natively. Like deno and bun, we can just import them.

@aryaemami59
Copy link
Contributor Author

@bradzacher Yes thank you!!!

designs/2024-support-ts-config-files/README.md Outdated Show resolved Hide resolved
designs/2024-support-ts-config-files/README.md Outdated Show resolved Hide resolved

The primary motivation for adding support for TypeScript configuration files to ESLint is to enhance the developer experience and accommodate the evolving JavaScript ecosystem. As TypeScript's popularity continues to grow, more projects are adopting TypeScript not only for their source code but also for their configuration files. This shift is driven by TypeScript's ability to provide compile-time type checks and IntelliSense. By supporting `eslint.config.ts`, `eslint.config.mts`, and `eslint.config.cts`, ESLint will offer first-class support to TypeScript users, allowing them to leverage these benefits directly within their ESLint configuration.

## Detailed Design
Copy link
Member

@bmish bmish Mar 12, 2024

Choose a reason for hiding this comment

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

How does the feature interact with the CLI option --config for specifying a config file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tested it, so far it seems to work pretty well actually, especially with v9. I'm probably going to write a bunch of tests as well to see if there are any edge cases but so far so good.

Copy link
Member

Choose a reason for hiding this comment

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

It would be good to explain what the behavior is when specifying TS or non-TS config files of varying file extensions through that option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't behave any differently, same as before. You can do eslint . --config=eslint.config.ts or eslint . -c eslint.config.ts and they just work. Same as with a eslint.config.js file.

Copy link
Member

Choose a reason for hiding this comment

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

Can you add that into the RFC?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it to the open questions, is that fine?

@ljharb
Copy link

ljharb commented Mar 13, 2024

Which version of TypeScript can the config file be written in? With what tsconfig settings?

Does this mean eslint would need to depend on typescript, causing it to be installed for non-TS users? if not, then would eslint be able to consume typescript in pnpm and yarn pnp in order to transpile the eslint config?

@bradzacher
Copy link

if not, then would eslint be able to consume typescript in pnpm and yarn pnp in order to transpile the eslint config?

It's possible to declare an implicit, optional peer dependency in a way that both yarn and pnpm will respect.

{
  "peerDependenciesMeta": {
    "typescript": {
      "optional": true
    }
  },
}

You don't even need to declare an explicit peer dependency with this config as it implicitly declares a dep on typescript: "*".

For context this is how the @typescript-eslint packages depend on TypeScript.

@aryaemami59
Copy link
Contributor Author

@ljharb

Which version of TypeScript can the config file be written in?

We are going to be using jiti to transpile the TypeScript config files. It has been used by frameworks such as Nuxt, Tailwind CSS and Docusaurus for this exact purpose. As far as I'm aware of, the only limitation it has, is that it doesn't yet support top-level await. The latest TypeScript syntax that I can think of that would also be relevant to our situation is the satisfies operator which I have made sure jiti has no problem with as I used it recently to do the exact same thing with size-limit.

With what tsconfig settings?

As far as I know, jiti does not care about tsconfig settings. It doesn't work like ts-node. I believe it uses babel internally to do the transpilling and it provides an interopDefault option to allow for using either export default and module.exports which in this situation would be very helpful as users will not have to worry about ESM/CJS compatibility issues.

Does this mean eslint would need to depend on typescript, causing it to be installed for non-TS users? if not, then would eslint be able to consume typescript in pnpm and yarn pnp in order to transpile the eslint config?

I think jiti will have to become either a dependency or an optional dependency. And we could make TypeScript an optional peer dependency as to not force the non-TS users to install it.

@mdjermanovic mdjermanovic added the Initial Commenting This RFC is in the initial feedback stage label Mar 19, 2024
Copy link
Member

@fasttime fasttime left a comment

Choose a reason for hiding this comment

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

LGTM. Pending re-approval of the latest changes by @nzakas.


const isTS = isFileTS(filePath)

if (isTS) {
Copy link
Member

Choose a reason for hiding this comment

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

We should probably do a simple test to see if Jiti is needed (it's not in Deno or Bun):

Suggested change
if (isTS) {
if (isTS && !globalThis.Deno && !globalThis.Bun) {

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

Overall, I'm happy with the direction. Let's make sure to note that jiti, tsx, or whatever we use, should be an optional dependency that people are required to manually install to use. I think there is still some exploration on implementation to do, but that's fine to do outside of the RFC as the overall approach won't change.

Given that we have two approvals from TSC members, I'm moving this to final commenting.

@nzakas nzakas added Final Commenting This RFC is in the final week of commenting and removed Initial Commenting This RFC is in the initial feedback stage labels May 17, 2024
@ljharb
Copy link

ljharb commented May 17, 2024

It's important to note that if a TS-specific tool is a runtime dep, then it's extra weight for non-TS users; and if it's a peer dep, even an optional one, some users may be forced to install it (not every tool considers optionality); and if it's an optional dep, it will always be installed (but if it fails to compile it won't fail the overarching install); and if it's none of those, then it's a hidden dep that users won't know is needed until runtime.

In terms of maintainability for eslint, and correctness for users, an optional peer dep is probably the best bet - but, note that adding peer deps, even optional ones, is a breaking change.

@miscellaneo
Copy link

It's important to note that if a TS-specific tool is a runtime dep, then it's extra weight for non-TS users

While Jiti would add a little under two MB for users, this is a significantly valuable feature, and adopting it in a way that is a breaking change would mean that it'd have to wait until ESLint 10. If ESLint 9's timeline was not an outlier, that will be quite a long time for users to wait. It may be preferable to add it as a dependency now, and make it a peer dependency for the next major version, so users will only be waiting for that potential 2 MB savings.

And, either way, I was under the impression that node_modules size is irrelevant.

@ljharb
Copy link

ljharb commented May 17, 2024

@miscellaneo i don't think your snark is helpful or in line with the CoC.

In general it's more that I don't want the ick of TS-related things in a project that doesn't suffer the burden of using TS, but I tried to express that in a way that wouldn't trigger those who love TS.

@silverwind
Copy link

silverwind commented May 17, 2024

even an optional one, some users may be forced to install it (not every tool considers optionality)

Which package managers don't honor peerDependenciesMeta.<dep>.optional? I think at least npm honors it.

And yes, i agree it must remain optional, keep eslint slim please.

@privatenumber
Copy link

peerDependenciesMeta.<dep>.optional is supported by these package managers:

npm: https://docs.npmjs.com/cli/v10/configuring-npm/package-json#peerdependenciesmeta
pnpm: https://pnpm.io/package_json#peerdependenciesmeta
yarn: https://yarnpkg.com/configuration/manifest#peerDependenciesMeta.optional

Personally, I think this is enough to move forward without a breaking change. But to be extra safe, I guess we should also investigate which versions of these package managers support for optional was added in, and see if those versions are still maintained / using a version of Node that's in LTS.

@ljharb
Copy link

ljharb commented May 18, 2024

@privatenumber it's still always a breaking change, because someone who has a version installed that doesn't satisfy the optional range will still get a failed install. Restating, adding a peer dep is always a breaking change, without exception, optional or not, forever.

@bradzacher
Copy link

doesn't satisfy the optional range will still get a failed install

@ljharb won't all the package managers either log a warning if it isn't satisfied (not fail the build) or just install the peer dep as specified?

I didn't realise that one of them hard failed if the peer dep wasn't satisfied.


If you wanted to avoid any logging etc whilst still satisfying pnp et al constraints then you can just not list the peer dep and only list the optional peer dep.

As I mentioned above - that's what we do for the TS dep for ts-eslint. We did that because of the install logs we caused for users who installed us as a transient dependency (specifically for create-react-app which always added our packages even if the user didn't need it).

@ljharb
Copy link

ljharb commented May 18, 2024

They should all fail - a peer del requirement is hoisted and necessary. Optional means it can be absent, not that it can be out of range.

@aladdin-add
Copy link
Member

@bradzacher as i can tell, npm has a --strict-peer-deps(defaults to false), setting to true can treat it as an error:
https://docs.npmjs.com/cli/v10/using-npm/config#strict-peer-deps

other package managers may have a similar config.

@bradzacher
Copy link

They should all fail

But they don't by default, as @aladdin-add notes.
I guess that's a question of the philosophy of breaking changes for eslint - is it a breaking change to add an optional peer dep?

That's not for me to decide as a non-member of eslint. I'd personally say no so long as the current/LTS versions of package managers silently ignore the optional dep (as @privatenumber mentioned above - will require some research to confirm that).

@ljharb
Copy link

ljharb commented May 18, 2024

That’s not what that config is for - that’s to disable npm’s attempt to guess the right version. Im pretty certain a mismatch will always fail.

@nzakas
Copy link
Member

nzakas commented May 20, 2024

We should include testing of the different behaviors of different package managers as part of the implementation before release.

My feeling is that adding an optional dependency that isn't installed by default doesn't constitute a breaking change, but it really comes down to the user experience.

@ljharb
Copy link

ljharb commented May 20, 2024

Testing is a great idea. In particular, i believe it’s a breaking change for a user who already has an out of range version of that optional peer dep installed - optional refers to presence, not range compliance.

@bradzacher
Copy link

One could declare an optional dep on * and then check the version at runtime (eg if and only if the package is used).

That's what we do on typescript-eslint for the typescript dependency. We do that to allow people to run older or newer TS versions at their own risk with a warning logged (that can be turned off via config).

https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/typescript-estree/src/parseSettings/warnAboutTSVersion.ts

@ljharb
Copy link

ljharb commented May 20, 2024

Fair point, that would make it pretty defensibly non-breaking.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

The overall approach LGTM. I agree to leave the details like which tool we will use (jiti, tsx, or something else that provides the functionality needed for this design) and how we will declare it as a dependency for the implementation phase.

@nzakas
Copy link
Member

nzakas commented May 24, 2024

This RFC has been approved. That means implementation can officially begin.

@fasttime as you've done a lot of investigation into compatibility of the different approaches, we'll look to you to help guide the implementation to completion.

@nzakas nzakas merged commit d2ce64f into eslint:main May 24, 2024
5 checks passed
@aryaemami59 aryaemami59 deleted the support-ts-config-files branch May 25, 2024 17:49
@silverwind
Copy link

silverwind commented Jul 24, 2024

FYI node just landed nodejs/node@35f92d9 which enables typescript execution 🎉.

@fasttime
Copy link
Member

FYI node just landed nodejs/node@35f92d9 which enables typescript execution 🎉.

Thanks for the update. If new versions of Node.js add stable built-in support for TypeScript files, I think we may consider removing the additional logic to support TypeScript configs in a future major release of ESLint.

@nzakas
Copy link
Member

nzakas commented Jul 25, 2024

It is implemented with an experimental flag, so we'll need to wait until it's stable to consider it.

@silverwind
Copy link

Yeah, it's going to be a while until that stabilizes. Maybe for Node 24.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Final Commenting This RFC is in the final week of commenting
Projects
None yet
Development

Successfully merging this pull request may close these issues.