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

New: First-Class Support For .eslintrc.ts #50

Closed
wants to merge 9 commits into from
Closed

New: First-Class Support For .eslintrc.ts #50

wants to merge 9 commits into from

Conversation

G-Rath
Copy link

@G-Rath G-Rath commented Nov 18, 2019

Summary

Support the ability to consume .eslintrc.ts configuration files natively.

Related Issues

eslint/eslint#12078
eslint/eslint#12082


This is a working implementation for this RFC (Confirmed working in IntelliJ w/o requiring any additional changes)
diff --git a/node_modules/eslint/lib/cli-engine/config-array-factory.js b/node_modules/eslint/lib/cli-engine/config-array-factory.js
old mode 100644
new mode 100755
index cf529b6..35595bd
--- a/node_modules/eslint/lib/cli-engine/config-array-factory.js
+++ b/node_modules/eslint/lib/cli-engine/config-array-factory.js
@@ -51,6 +51,7 @@ const eslintRecommendedPath = path.resolve(__dirname, "../../conf/eslint-recomme
 const eslintAllPath = path.resolve(__dirname, "../../conf/eslint-all.js");
 const configFilenames = [
     ".eslintrc.js",
+    ".eslintrc.ts",
     ".eslintrc.yaml",
     ".eslintrc.yml",
     ".eslintrc.json",
@@ -194,6 +195,39 @@ function loadJSConfigFile(filePath) {
     }
 }
 
+/**
+ * Loads a TypeScript configuration from a file.
+ * @param {string} filePath The filename to load.
+ * @returns {ConfigData} The configuration object from the file.
+ * @throws {Error} If the file cannot be read.
+ * @private
+ */
+function loadTSConfigFile(filePath) {
+  debug(`Loading TS config file: ${filePath}`);
+
+  let registerer;
+
+  try {
+    registerer = require('ts-node').register();
+  } catch (e) {
+    debug(`Could not register ts-node to read TypeScript file: ${filePath}`);
+    e.message = `ts-node is required for TypeScript configuration files. Make sure it is installed\nError: ${e.message}`;
+    throw e;
+  }
+
+  try {
+    registerer.enabled(true);
+    const imported = importFresh(filePath);
+    registerer.enabled(false);
+
+    return imported;
+  } catch (e) {
+    debug(`Error reading TypeScript file: ${filePath}`);
+    e.message = `Cannot read config file: ${filePath}\nError: ${e.message}`;
+    throw e;
+  }
+}
+
 /**
  * Loads a configuration from a package.json file.
  * @param {string} filePath The filename to load.
@@ -250,6 +284,9 @@ function loadConfigFile(filePath) {
         case ".js":
             return loadJSConfigFile(filePath);
 
+        case ".ts":
+          return loadTSConfigFile(filePath);
+
         case ".json":
             if (path.basename(filePath) === "package.json") {
                 return loadPackageJSONConfigFile(filePath);

@jsf-clabot
Copy link

jsf-clabot commented Nov 18, 2019

CLA assistant check
All committers have signed the CLA.

@G-Rath
Copy link
Author

G-Rath commented Nov 18, 2019

@kaicataldo There you go :)

@bradzacher I've attached links to your works, and used a lot of your comments - thanks a ton for your help, and especially for your playground code; I think that's a solid example for how to explore typing eslint plugins.


I think I've captured the majority of things. I've mentioned but also leaned away from the type definition side of things, b/c I think they're a natural next step but technically not actually what this RFC is about?

That might not matter, since it's one of the biggest reasons for why to accept it, but 🤷‍♀ it's my first RFC 😂

Let me know any changes, questions, input etc

I've included a patch of an implementation that I've confirmed works perfectly as I'd hope w/ IntelliJ & @typescript-eslint/parser.

@mysticatea mysticatea added Initial Commenting This RFC is in the initial feedback stage and removed triage labels Nov 18, 2019
@mysticatea
Copy link
Member

mysticatea commented Nov 18, 2019

Hi. Thank you for your contribution!

Well, because the purpose of the RFC process is to clarify user-facing changes and to request feedback, the "Detailed Design" section should describe the user-facing changes rather than the implementation changes. I guess that most people cannot know the user-facing changes from the implementation description.

Therefore, would you update the "Detailed Design" section with the changes from the user perspective? I guess that the design of this RFC is the following stuff:

  • Adds the support for .eslintrc.ts.
  • People require to install ts-node manually to use .eslintrc.ts (where should people install it to?). Otherwise, ESLint stops with a fatal error at .eslintrc.ts.
    • I think it needs an understandable error message and instruction.
  • ESLint doesn't validate the type of the exported object for now.
  • (what is the priority?)

@G-Rath
Copy link
Author

G-Rath commented Nov 18, 2019

@mysticatea thanks for the feedback!

I've updated the RFC per your comments. Let me know if there's anything else 🙂

Copy link
Member

@mysticatea mysticatea left a comment

Choose a reason for hiding this comment

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

Thank you for the fastly update.

designs/2019-typescript-eslintrc-support/README.md Outdated Show resolved Hide resolved
designs/2019-typescript-eslintrc-support/README.md Outdated Show resolved Hide resolved
@G-Rath G-Rath requested a review from mysticatea November 18, 2019 09:52
Copy link
Member

@mysticatea mysticatea left a comment

Choose a reason for hiding this comment

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

This proposal sounds good from my perspective. It's worthful if people can use .eslintrc.ts in the same manner as .eslintrc.js because ESLint is supporting TypeScript via @typescript-eslint and the community is migrating to ESLint from TSLint.

@ljharb
Copy link

ljharb commented Nov 18, 2019

How will this interact with differing TypeScript versions? How will eslint be sure to work with TS 1, 2, 3, and 4+, without requiring version bumps?

Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

My biggest concern is the additional maintenance burden. This doesn't seem like a particularly useful feature unless type definitions are included, and if the community doesn't implement it, the core team will have to. It also will create additional maintenance burden for 3rd party plugin authors.

That being said, I do think it could be beneficial for the team to maintain definition files in core for public APIs (including configuration files and anything we expose publicly in the Node API).

designs/2019-typescript-eslintrc-support/README.md Outdated Show resolved Hide resolved
designs/2019-typescript-eslintrc-support/README.md Outdated Show resolved Hide resolved
@bradzacher
Copy link

How will this interact with differing TypeScript versions? How will eslint be sure to work with TS 1, 2, 3, and 4+, without requiring version bumps?

Isn't that the point of relying upon ts-node?
ESLint calls ts-node, and then ts-node will use whatever compatible TS version that's been installed by the user - which means that eslint won't be tied to a specific version.
There's ofc the risk that ts-node changes its require API, but that risk is pretty close to zero, as they rely upon that mechanism for users to use ts-node with cli tools like mocha: mocha --require ts-node/register, and that API is based off of how the nodejs api works IIRC.

In regards to version compatibility with types; as described by the RFC, the types will be maintained by the community (like they are right now).
The @types/eslint package is tied to a minimum typescript version - which is currently tied to a minimum ts v3.1. For reference 3.1 is just over a year old (released Sept 27 2018), and in my experience, most teams are somewhere around this version. It's rare that I have seen users on much older than that, as most tooling requires around that.

My biggest concern is the additional maintenance burden... and if the community doesn't implement it, the core team will have to.

Speaking as the maintainer of @typescript-eslint, I would definitely be interested in creating tooling as part of that project if this were to land.

IMO there is also a simple, incremental support model for the types that will work fine, and will enable users to get some checking, and have plugin authors opt in to adding support if they want to.
Example types

Failing that - the community can maintain types for plugins; I mean this is exactly the way in which the DefinitelyTyped repo works and exists - package authors don't have to implement types because the community implements them as required. When a package decides it wants/is able to take on the work to maintain the types, they move them internally.

A good example of a major package which relies upon this model is react. The react team does not provide any typescript types (or flow types, even though their repo is written in flow!).
The community has built, and maintains the react typescript types (the flow team maintains the react flow types, but that's a separate thing).

@ljharb
Copy link

ljharb commented Nov 18, 2019

so does that mean that eslint would need a dep or a peer dep on ts-node?

@bradzacher
Copy link

so does that mean that eslint would need a dep or a peer dep on ts-node?

To be strict, and keep package managers happy, you would probably want to define an optional peer dependency.

@ljharb
Copy link

ljharb commented Nov 18, 2019

That’s only a very new feature in npm; in older npm versions all peer deps are always required.

@ljharb
Copy link

ljharb commented Nov 18, 2019

In other words, this seems like something that should be done via a separate package, and not a dep forced on all non-TS users of eslint.

@G-Rath
Copy link
Author

G-Rath commented Nov 18, 2019

so does that mean that eslint would need a dep or a peer dep on ts-node?

No it does not - case & point: the majority of the "Prior Arts" don't have ts-node or TypeScript as a peer dep and they've had it that way for a long time w/o issue.

To be strict, and keep package managers happy, you would probably want to define an optional peer dependency.

It could be nice to have yes, but I don't think it's a strict "must-do" - ts-node itself is a very stable package, and one that generally shouldn't be directly dependent for the same reason TypeScript shouldn't, so leaving it out of peer dep shouldn't be the end of the world.

That’s only a very new feature in npm; in older npm versions all peer deps are always required.

And they might do again; meanwhile afaik yarn never installed peer deps by default.

In other words, this seems like something that should be done via a separate package

Use of a separate package would require the changing of a lot of tooling & editors to ensure that they support two types of ESLint, where as this proposal will require no change to tooling & editors to support.

@ljharb
Copy link

ljharb commented Nov 18, 2019

@G-Rath so the mere presence of ts-node on disk magically enables this behavior? that seems highly problematic and implicit.

npm hasn't installed peer deps by default since v3, but peer deps have always been conceptually required regardless of auto-install behavior. peerDependenciesMeta is a brand new way to loosen this requirement, but only for CLIs that understand it. What npm might do in v7 is auto-install peer deps - but they remain, forever, required, not optional (absent peerDependenciesMeta).

@G-Rath
Copy link
Author

G-Rath commented Nov 18, 2019

@ljharb

so the mere presence of ts-node on disk magically enables this behavior?

The presence of ts-node in the appropriate node_modules folder, as per standard Node module requiring behaviour.

I would call it no more magical than being able to call eslint from .bin w/o actually needing to do node_modules/.bin/eslint, or prefix it w/ node.

peer deps have always been conceptually required regardless of auto-install behavior.

Then don't have them as peer dependencies.

However, I'm not sure about them always being conceptually required? Quoting from the npm documentation:

https://docs.npmjs.com/files/package.json#peerdependencies

In some cases, you want to express the compatibility of your package with a host tool or library, while not necessarily doing a require of this host. This is usually referred to as a plugin

This ensures your package tea-latte can be installed along with the second major version of the host package tea only.

Bold emphasis mine - the use of "can" & "along" to me implies that they are optional 🤷‍♀

It's a not a smoking gun for sure, but I also don't get the opposite impression from that section of the documentation either.

They do say this however:

Notably, your module may be exposing a specific interface, expected and specified by the host documentation.

Which could arguably be a case in favor of not specifying it as any dependency, since the host (ts-node) is not expecting any particular interface from ESLint.

@bradzacher
Copy link

so the mere presence of ts-node on disk magically enables this behavior? that seems highly problematic and implicit.

IMO it's no more magical than eslint automatically picking up .eslintrc.js and .eslintrc.yml.
And no less magical than eslint automatically figuring out whether your .eslintrc file is json or yaml (deprecated functionality, but still documented).

How do you know exactly how eslint works in these scenarios? Well everything is clearly documented in the docs: https://eslint.org/docs/user-guide/configuring#configuration-file-formats

Following on from that, to me it seems like your concern is solved via a line or two in the above documentation section?

 ## Configuration File Formats

 ESLint supports configuration files in several formats:

 * **JavaScript** - use `.eslintrc.js` and export an object containing your configuration.
 * **YAML** - use `.eslintrc.yaml` or `.eslintrc.yml` to define the configuration structure.
 * **JSON** - use `.eslintrc.json` to define the configuration structure. ESLint's JSON files also allow JavaScript-style comments.
 * **Deprecated** - use `.eslintrc`, which can be either JSON or YAML.
 * **package.json** - create an `eslintConfig` property in your `package.json` file and define your configuration there.
+* **TypeScript** - use `.eslintrc.ts`, and export an object containing your configuration. Also ensure that the `ts-node` package is installed and available to your eslint installation (i.e. locally if running locally, or globally if running globally).

 If there are multiple configuration files in the same directory, ESLint will only use one. The priority order is:

 1. `.eslintrc.js`
 1. `.eslintrc.yaml`
 1. `.eslintrc.yml`
 1. `.eslintrc.json`
+1. `.eslitrc.ts`
 1. `.eslintrc`
 1. `package.json`

@ljharb
Copy link

ljharb commented Nov 18, 2019

It's more magical than config files because ts-node might be in my node_modules folder through no action of my own - whereas config files are there because I put them there.

Jest does this same thing, where the presence of a package magically enables babel registration, and it causes no end of headaches.

My concern is that nothing should change without explicit user action. A CLI flag or env var, for example, would be fine.

@G-Rath
Copy link
Author

G-Rath commented Nov 18, 2019

whereas config files are there because I put them there

You also made the choice to write your configuration file in TypeScript, since otherwise this is a non-issue as the RFC doesn't call for ESLint to run ts-node on configuration files that don't end w/ .ts if ts-node is around.

My concern is that nothing should change without explicit user action

How about the act of explicitly changing the extension of the configuration file? 😉

A CLI flag or env var, for example, would be fine.

I've detailed in the RFC why a flag or env variable would be a poor middle-ground: they'd make it harder to use out of the box w/o changes to tooling & editors (just if you missed that 🙂).

@bradzacher
Copy link

bradzacher commented Nov 18, 2019

I fail to see the problem with it "just working" in that case.
Explicit opt-in is required to even lint typescript files via the --ext .js,.ts flag, and this causes much pain and friction for typescript users trying to onboard onto eslint.
It's not good friction that makes users think about anything - it's an annoyance that makes everyone copy "magical working commands" around the place because it worked in once place.

This would be the same - people don't want to have to create the .ts file, and then modify all of their package.json scripts so that they now also add an additional --use-ts-node-yes-i-really-mean-it flag. It is just unnecessary friction.

To me, the fact that it works "magically" if some other package has installed ts-node seems perfect, as long as the case where it's missing is explicitly messaged, so if said package drops the dep, the user is clearly alerted, and can easily can figure it out how to fix it. i.e

  • eslint detects a .eslintrc.ts and there is no other .eslintrc in the directory (i.e. give .eslintrc.ts the lowest priority, above package.json).
  • eslint logs a debug message: "requiring ts-node from local package"
  • eslint fails to find ts-node:
    • eslint stdout logs: "It looks like you're using a .eslintrc.ts file, but have not got the ts-node package installed. You must install it to enable .eslintrc.ts support".
    • eslint throws and exits (unless I'm mistaken, this is the same as it would do if there was a syntax error within a .eslintrc.json etc file).
  • eslint finds ts-node:
    • eslint logs a debug message: "resolve ts-node to /node_modules/ts-node, will use this to parse and compile the .eslintrc.ts file"
    • eslint continues as expected.

It seems like a lot of your concerns is things working magically with no visibility when things go wrong, which (as shown) should be possible to handled by using clear debug messages when it's all good (so it's quiet normally, but can be inspected if required), and clear error messages when it's not working.

@mysticatea
Copy link
Member

@ljharb To clarify. This proposal does nothing even if just ts-node exists in node_modules. This doesn't change anything for JS users. Only if people did the explicit action that makes .eslintrc.ts, ESLint loads ts-node and enables the ts-node only while loading the .eslintrc.ts.

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.

Thanks for all of the work on this. I think it's an interesting proposal but not one I can support. The primary reason is that I don't think it's a good idea to build TypeScript directly into the ESLint core. I've never wanted ESLint to be a kingmaker in the JavaScript ecosystem, where things that get into ESLint get considered "official" or "supported". If someone were to ask why we can't have Flow support built-in, I wouldn't know how to differentiate between a decision to include TypeScript and a decision to include Flow, never mind any other dialects that might pop up in the future.

That said, I can see that there is a non-trivial number of ESLint users who would like a way to use TypeScript for their ESLint configuration, but rather than bake this into ESLint itself, I think we should instead look at some kind of generic plugin mechanism where a plugin can define an alternate way of providing configuration to ESLint. That way, the typescript-eslint project would be able to provide a plugin to do what it wants and while the core of ESLint remains agnostic.

@G-Rath
Copy link
Author

G-Rath commented Nov 21, 2019

@nzakas thanks for taking the time to read and weight in on this RFC, and I respect your stance, as it's a valid concern.

My only two counter-comments is that there is no "flow-eslint" in the same way there is @typescript-eslint. There is a whole ESLint community running parallel to the core that supports this language - that's how I differentiate between this, and other languages.

In addition, while I totally think that plugin system is the right long-term solution, it really doesn't help us in the short-term, as it's the sort of thing is a large change, and requires a much larger RFC.

Those are my thoughts, but again, completely respect and understand your position - thanks again for taking the time to weigh in 🙂

@nzakas
Copy link
Member

nzakas commented Nov 21, 2019

@G-Rath Sure thing. Please keep in mind that we, on the core team, really do need to think long-term even if it means some short-term inconvenience. Things change rapidly in the JS ecosystem and it's a big job to keep ESLint stable enough to continue to function in that kind of environment.

I've been interested in creating some kind of a pluggable config system for a while now (built off of #9), so this could be a good reason to move forward on those discussions.

@G-Rath
Copy link
Author

G-Rath commented Nov 21, 2019

@nzakas Definitely understand having to think about the long-term, but when do you consider something to be a long-term problem?

The RFC you linked was opened just under a year ago, and seems to continue to be hotly debated with little sign of a resolution on the horizon - there also seems to be outstanding problems that still need to be resolved.

That's my impression from a brief look, and I'm new to the core side of ESLint, so it could be possible that I'm off-base here, but as you say:

Things change rapidly in the JS ecosystem

So it's a big deal to be told that this RFC could easily be delayed for months or possibly another year in the hope that it'll be handled by another catch-all RFC, given that the diff of the implementation is very small and very stable - it's the same implementation that's already being used in a number of packages that are in very similar positions to ESLint (i.e Kama, Jest, Webpack).

This also means that it's very easy to undo - if and when a new configuration system comes about, or when a new something seems to get enough movement behind it and that there could be value for supporting configuration files written it (b/c i.e there isn't really a gain to writing your configuration files in coffeescript, as linting isn't really something you have conditional settings on), then superseding this RFC w/ another, and reverting its implementation to be replaced by something else is a very easy thing to do 🙂

So when you say:

some short-term inconvenience

This inconvenience has been around for a long time; just how long is arguably, but still it's been an inconvenience for at least a year if not more:

  • TSLint was deprecated at the start of this year, in favor of ESLint
  • The @typescript-eslint community has been around for at least a few years prior to the deprecation of TSLint
  • JSONSchema validation messages have been giving people grief since pretty much the day ajv came about,
  • Given the nature of people, I'm sure that users have been having "trouble" with eslint configuration since the day it supported configuration.

While generalisations, these stretch back years - at what point does it stop being short term, and become long term?

I grant it's not a pressing high priority problem, nor is it one that people have been screaming out for, but I think a big part of that is b/c no ones submitted an RFC until now, and just accepted it as fact that we can't using TS files meaning we've not bothered exploring what cool things we could do.

@bradzacher said as much in his reply to my original issue:

This is an interesting thought. I've considered it before, but I don't hugely want to deviate from eslint with a cli wrapper.


So while I completely understand, and am in full support of finding a nice pluggable means of supporting whatever language people wants, I feel in this case the "what about flow & co" argument is a bit of a bikeshead, as the area of overlap in the venn diagram of "languages that can be linted by eslint" vs "languages that benefit having your eslintrc written in" is rather small 🙂

@mysticatea
Copy link
Member

@nzakas I understand your concern, but... How does the configured plugin change the way that ESLint loads config files? ESLint doesn't know what plugins are configured until it loaded configs, but the behavior of this RFC is needed before it loads configs because it's about how it loads configs.

@nzakas
Copy link
Member

nzakas commented Nov 28, 2019

@G-Rath I completely understand your point, but I think you took my comments a bit too literally. :) To me, the only path forward is to come up with some way that different config file formats can be supported from outside of the ESLint core. I thought implementing #9 could make doing such a thing easier because it limits the number of places configuration can take place, but it's probably not a prerequisite

@mysticatea I don't know. Maybe it would have to be a different type of plugin for ESLint than we currently have. I really didn't think it through. My main point is that this isn't something that should be included in the core, but I'd support having a way for third-parties to create ways to do similar things.

@G-Rath
Copy link
Author

G-Rath commented Dec 19, 2019

Given that the last comment on this was ~20 days ago, I think this is ready for final commenting.

The comments that remain unresolved are b/c I didn't want to mark them as such w/o the OC being happy w/ my responses.

On an side, there is an active open PR to allow ts-node to allow configuration to be read from tsconfig.json, which will further reduce potential burden on eslint.

@nzakas
Copy link
Member

nzakas commented Mar 6, 2020

I don't think there is enough support on the team for us to consider incorporating this. Again, I appreciate the thought and time put into the RFC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Initial Commenting This RFC is in the initial feedback stage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants