Investigating ESLint migration #300
Comments
|
Hi! Sorry it took a while for us to respond. We've also been thinking about this off and on, and are definitely open to getting it working and replacing tslint! I've pushed a new branch which we can work towards: https://github.com/microsoft/dtslint/tree/eslint_base |
|
do you want me to open a draft PR against that? its been a while so ill catch it up from master too while im in there. then we can decide if the stuff i already did is worth using as a starting point or not |
|
Yeah, that'd be great! |
|
👋 is anything happening here? I'd be interested in pitching in! |
|
@JoshuaKGoldberg it is being tracked in #325 by @sandersn , although he's presumably been preoccupied with other stuff recently. But reviewing that pr would be a good start I think |
|
#325 is stale as sanders doesn't currently have the time to push ahead with it, so any takers for opening a new PR would be great. some feedback from me:
|
|
Yeah I'm planning on looking at this within the next week or so. My rough thoughts are that it would be cleanest long term to promote From the perspective of DefinitelyTyped it would likely be very difficult to immediately switch from ESLint to TSLint given the sheer scale of the codebase and how many equivalent rules have subtle differences between the two linters. My first guess for a strategy would be something like:
...but that's just a rough guess that should be validated 😄 |
|
keep in mind dtslint has plenty of useful rules beyond type assertions. that is actually only one rule, a small amount of the source of dtslint. if there's enough complexity in that one rule to justify pulling in third party libs, fair enough. the rest can mostly be converted fairly easily (but time consuming). so it would be super nice if we could have them running alongside each other which i think is what you're saying. so we can migrate a rule at a time, leaving the rest to continue using tslint. i also already have a branch with some of those rules converted but none of the surrounding architecture updated to call them properly. so let me know if you want any of that in your efforts |
|
I've reached out to @ibezkrovnyi to talk about |
|
I don't know the eslint naming scheme, but the rules are really style rules for Definitely Typed. I'd say that (at least) a few of them don't make sense outside that context, like the lint rules about when to use |
|
Maybe two packages then, My main goal here is extracting the parts of dtslint that are broadly applicable into a standard ESLint plugin. My secondary goal is making dtslint use a typical ESLint plugin architecture so it can be made portable. |
|
Yeah I mean there's a good amount of purely dt-specific rules that should continue to live in this repo (even if this repo becomes an eslint plugin). But it's true a couple of them could be useful to other people, through even then someone could just use this plugin (eslint-plugin-dts for example, we could call it). I've also converted a fair few of the rules already to eslint rules. if someone gets time to convert the architecture in general I could easily pr them in |
|
FWIW I've taken on ownership of Edit: note that eslint-plugin-expect-type is on the Apache-2.0 License. I'll make sure we do whatever we need to not violate that, such as attribution/permission. |
|
so would the plan be to update dtslint to make use of the expect-type plugin? given that it has many more DT-specific rules that don't belong in such a plugin (but rather in a DT-specific plugin, like this repo right here) |
|
Tentatively, my proposal is that we convert this repo into a monorepo containing at least three packages:
In theory we could use the separate
|
|
that makes sense i think, especially maintaining this repo and just shuffling it around. i already converted a fair few of the rules to eslint rules roughly (untested) so would be happy to contribute those if we did make some kind of i think however we reorganise it, though, the big job is migrating dtslint to run eslint rather than tslint. the rules are already essentially lint rules, so pulling those out into a separate package isn't a blocker to migrating the core imo. may be worth starting to look at that before anything else |
|
Poking through #325's changes and some of the adjacent areas of code, there are a few logic paths in
cc @sandersn |
I like the monorepo idea, although it's a bit of work to set up. Unless it would be tremendously difficult, it'd be better to keep it here and replace the existing structure. My main piece of advice, having recently upgraded all of DT at once to support exactOptionalPropertyTypes, is to make the transition incremental. Have dtslint run both tslint and eslint, and switch one rule over at a time. That'll make it easier to alternate between switching a lint rule and making sure all the DT packages still pass. |
|
Update: @andrewbranch and I talked about problems we've had updating dtslint with the rest of the Our motivation is the dependency Does that make sense? @JoshuaKGoldberg it's basically what you're doing, but even more so. |
|
Hi! Sorry I haven't had much time recently to respond (thanks for the ping andrewbranch). I think I understand the general direction of the proposal, but just clarifying... @sandersn if you wouldn't mind humoring me, a few clarifying questions...
Instead of moving dtslint into DT, an alternative could be to move the DT pieces dtslint uses into dtslint, and import them in DT. For the thread, right now dtslint takes in the following from
Do you have an opinion moving those to dtslint so the dependency chain becomes something like Speculating / posting for visibility: now that the core of dtslint is being moved to ESLint plugins, and most consumers likely have typescript-eslint setup anyway, maybe that community-first dtslint isn't a worthwhile goal? Is that the conclusion we should make here? Should the recommendation be to use a package like eslint-plugin-expect-type? My big hope here was that we could make dtslint into a community-first tool akin to ts-prune or typescript-eslint (as opposed to DT-first), where the entirety of the core tool is in a general open source repo. The benefit here is that the community gets a first-class package for what would end up being a package that can install & run the necessary ESLint+typescript-eslint+plugin(s) to lint .d.ts files. In that world anything DT needs on top of it would exist in the DT repo alone -- which sounds like a very nice separation of concerns? |
|
dtslint contains rules only useful to DT, though. There's only maybe one or two rules appropriate for a "community" plugin. It still is a tool specific to DT and exists to help maintain that repo and nothing else. I feel like because of that, dtslint should continue to exist whether it's in DT repo or not. Even if you split out the expect type rule, that's a really small part of a bigger tool. |
|
The way I think of dtslint today is this:
Notably, (2) is the same as (1) except that you want to pass a repo path that's not Considering (3), I think linting and testing d.ts files is a good idea, but I suspect that with If we started with (3) as a new package, DT might be able to use it as a basis when it was finished. My two concerns are (1) whether a general set of rules would be useful on DT (probably yes?) and (2) whether fixes we need would be easy enough (they aren't too common, so probably yes?). In summary: Writing all that clarified my thinking a bit: I'd recommend either
(1) followed by (2) also makes sense. [1] Yes! I can't believe that's a word. |
|
I have to admit I’m skeptical that dtslint has a bright future as a community-first tool. It definitely has some rough edges, and if the community wanted to adopt the project, they would rightly want to make a lot of changes to smooth them out. At the same time, having it in a separate repo from the rest of DefinitelyTyped-tools, even one under full Microsoft control, is creating a maintenance burden now. Personally, I think these concerns are countervailing forces and both parties would be better served by making a clean break, encouraging new community projects to be built on ESLint and maintained outside of DT/Microsoft. People who are currently depending on dtslint can continue to use it, but its original microsoft/dtslint repo will encourage them not to, and it will be built out of microsoft/DefinitelyTyped-tools instead. That’s my recommendation—admittedly biased because it’s the most convenient path forward for me personally, but I feel like it makes sense. Thoughts? |
|
I think i agree.. To me it makes most sense that we migrate the runner from tslint to eslint and migrate the rules (one at a time if possible, but all at once otherwise). The biggest piece of work there is migrating the runner, i would say and maybe some of the more complex rules. Moving that to definitelytyped-tools or some other appropriate DT repo makes a lot of sense too , and doesn't change how the migration happens really. the expect-type rule being an eslint rule others can use makes sense but dtslint contains a lot more rules than that, most of which are very specific to the DT repo. its very unlikely anyone else would ever want to use them unless they had a private DT-like repo. thats why i think even if we extract the expect-type rule, we should stick to keeping dtslint (or an equivalent) inside a DT repo and treated as an internal maintenance tool. |
|
I feel a linter shouldn't be used for testing language features and source code logic. |
|
I know at one point we were suggesting people use https://www.npmjs.com/package/tsd, but I haven’t heard much about it lately. |
|
👍 I have been convinced, thanks for the detailed explanations! Keeping DT specific runtime to the DT repo makes sense. Since there's a clear path forward for moving into DT my plan is to stop sending PRs to this repo & DT that split out functionality, and instead focus on eslint-plugin-expect-type. LMK if there's anything else I can do to help! (do you still want assistance in moving to ESLint internally given it's moving to the other repo too?) |
Yes, although it makes sense to move dtslint into DefinitelyTyped-tools first (in my opinion), and a completed eslint-plugin-expect-type might replace dtslint's expectRule. If nobody else gets to it first, I'll probably work on the dtslint move during my next DT rotation, some time in November I think. |
|
Hi, guys. Newbie question here. I saw some PR with title PS: I used |
|
typescript < 4.5 https://www.npmjs.com/package/eslint-plugin-dtslint $ tail dtstest.ts dtslint.eslintrc.cjs tsconfig.json
==> dtstest.ts <==
const answer = 42; // $ExpectType 41
==> dtslint.eslintrc.cjs <==
const { join } = require("path");
module.exports = {
parser: "@typescript-eslint/parser",
parserOptions: {
ecmaVersion: 2019,
project: join(__dirname, "./tsconfig.json"),
sourceType: "module"
},
extends: ["plugin:dtslint/recommended"],
};
==> tsconfig.json <==
{
"extends": "@tsconfig/node14"
}
$ eslint -c dtslint.eslintrc.cjs dtstest.ts
1:1 error Expected type to be: 41; got: 42 dtslint/expect-type
✖ 1 problem (1 error, 0 warnings) |
Hello 👋
Recently, out of interest, I made a branch of dtslint which drops tslint entirely and uses ESLint instead.
TSLint has been obsolete for a very long time, and deprecated for quite some time too. It would be good to move towards removing it IMO, is that something you're interested in?
I'm happy to continue my branch if that's the case, but it would be very nice to have some help converting the individual rules as there are many and I haven't touched this repo before.
Couple of notes from what I've found so far:
{start, length}which is aligned with TSLint but not ESLint. In ESLint we need a{column, line}or aNodeIf you're interested in seeing my branch, i could make a draft PR which doesn't yet build and leave comments where I'm unsure what to do (usually vague types that were previously
anyor inferred).The text was updated successfully, but these errors were encountered: