feat: parsing session objects

[JoshuaKGoldberg]

Summary

Parsers are not currently told whether ESLint is being run in a CLI-style single pass, a single pass with --fix enabled, or a long-running IDE session.
This RFC proposes the creation of a "session" object that provides them this information.

Related Issues

• Complete rewrite of ESLint eslint#16557
• Single run detection for type-aware linting typescript-eslint/typescript-eslint#3528

[JoshuaKGoldberg]

Aside: this is why I filed Change Request: Rename internal CLIEngine to ESLintEngine or similar? #16670. A little confusing to work on this RFC 😄

[JamesHenry]

Looks good to me!

@dbaeumer do you have any specific thoughts or concerns with regards to the VSCode extension?

[nzakas]

Thanks for putting this together. As I mentioned inline, I kind of feel like we are slapping more functionality on top of a bad interface. It's my belief that stateful ESLint parsers only exist because parsers are the only plugins that interact with the core in a bidirectional way. Knowing what we know now, that doesn't feel like the right model, so I'd prefer not to continue building on top of it.

I'd much rather think about how plugins could be created to pass useful information like this outside of the parse sequence.

[nzakas]

I think there is a larger question here about whether parsers should actually be stateful, or if this is a side effect of a poorly-designed integration.

I'm leaning towards the latter.

[JoshuaKGoldberg]

Sure, that makes sense. I'd +1 that it feels off to be putting stateful long-lived logic in a parser.

But, if this whole area of ESLint is slowly getting sunset in favor of the new config, why not non-destructively add to it now? Again, it's going to be quite a while before most users are able to use the new rewritten parts. Not touching this old system now traps them in a sub-optimal linter setup.

[nzakas]

It's very easy to say "keep adding things to the ship that's sinking because it won't hurt" but that's not actually true. Anything we add needs to be maintained for some period of time, and that means pulling development time away from other things.

Additionally, we don't want to implement something we know isn't the best solution, have people start to rely on it, and then say "sorry, that's not supported" in the next version. I'd rather come up with what we know to be the solution for the future than keep adding bandaids that we might not want or need in the future.

[JoshuaKGoldberg]

A small point: it would be easier for us to figure out structural needs around performance if we could run with this in production for a bit to gather user feedback.

Additionally, we don't want to implement something we know isn't the best solution, have people start to rely on it, and then say "sorry, that's not supported" in the next version.

We know that no matter what the new world looks like, stateful logic (as currently implemented in the eslint-plugin-import and typescript-eslint parsers) will need to know what mode ESLint is running in. I'll go ahead and reduce the RFC's scope to just providing a single mode indicator that includes knowledge on whether --fix is enabled.

[nzakas]

I think we need to look at another alternative that doesn't involve hacking more functionality into parseForESLint().

[JoshuaKGoldberg]

If we do find an alternative fitting the description, would you accept it in the existing ESLint setup? Or is this comment directed only for the upcoming rewrite?

[nzakas]

If we can define an alternative that makes sense for the future, there's no problem with finding a backport to the current version of ESLint.

[bradzacher]

I'd much rather think about how plugins could be created to pass useful information like this outside of the parse sequence.

For eslint-plugin-import, the stateful information is indeed calculated by the plugin, but currently it's calculated during the lint step because there's a dependency on having a file's AST (traverse AST -> calculate dependencies -> calculate state from dependencies).

However for @typescript-eslint it's the parser that calculates the stateful information for @typescript-eslint, not the plugin. The @typescript-eslint/parser produces it and then any plugin in the ecosystem can freely consume the type information, regardless of whether or not @typescript-eslint/eslint-plugin is even installed.
Currently type information is provided to rules via parser services (context.parserServices). We will need to ensure that any changes we make do not change this contract, or else we would break the ecosystem in a way that would cause much pain and friction for users.

Additionally it's worth noting that for @typescript-eslint the stateful information and the AST are intrinsically linked. By computing the type information for file A, TS has to first parse file A and produce a TS AST for it - which we then use to create the ESTree AST (improving performance by de-duplicating work).

---

On the surface I don't have an issue if we were to introduce a new concept into the tooling (like a "state manager") as long as it's possible for users using non-flat ESLint configs to get for free. IMO It would be a folly to require users to make config changes or migrate to flat configs to take advantage of the changes proposed in this RFC because a lot of users don't read release notes and even more users will actively resist making config changes.

We should strive to keep this entirely transparent to users.

The reason that we really liked this design is because:

• It is a transparent additive change that stateful parts of the ecosystem can choose to consume to optimise their management in newer versions of ESLint.
• It's easily backwards compatible: if the data doesn't exist then the fallback assumption is for the system to use the "persistent" style.
• Unlike current workaround (inferring "single run" via environment checks), custom lint run tooling can opt-in to the behaviour.
• It works for both stateful parsers and for stateful plugins.

[nzakas]

The @typescript-eslint/parser produces it and then any plugin in the ecosystem can freely consume the type information, regardless of whether or not @typescript-eslint/eslint-plugin is even installed.

That's interesting -- I kind of forgot about that. But to play devil's advocate, does it have to work that way? It seems like we could potentially get more bang for the buck by creating a plugin solution that encapsulated parsing, type information, and rules.

it's possible for users using non-flat ESLint configs to get for free

Unfortunately, this just isn't possible. We can't keep tacking things on to eslintrc and expect to be able to make the move to flat config. Any new options are going to be flat config only, and that's okay, because in the not-so-distant future flat config will be the only config.

[bradzacher]

because in the not-so-distant future flat config will be the only config

That's great for people who are on the latest version (presumably at that point, v9.x?).
But what about the millions upon millions of users that aren't?

Breaking down the stats from NPM - ESLint's 29.8m weekly downloads are distributed across major versions like this:

Major	Weekly DL #	User %
0	7,124	0.02%
1	28,621	0.09%
2	240,135	0.81%
3	349,503	1.17%
4	880,653	2.96%
5	1,255,334	4.22%
6	2,588,353	8.69%
7	8,325,109	27.96%
8	16,103,704	54.08%

Just over half the users have migrated to v8 - which is a not insignificant portion!
For me, however, the troubling number is that a quarter of users are still on v7 (ignoring the other quarter on even older versions). It's been 1.25 years since v8.0.0's release - so by this point many of the stragglers have made the jump to v8 - likely because the v7->v8 jump was pretty low-breakage for most users.

v9 may be out soon™️ - but there will be a significant percentage of users that trail behind for many months. I would hazard a guess that there will be more people that delay "taking the jump" to v9 because it will require them to rewrite their config file - it will likely be a year or more before a significant portion of users are on v9.

I do understand the desire to put a hard stop on the old config system and solely focus on the new, but should we be doing that at the cost of the user's experience? Wouldn't it be better if users could do a non-breaking bump npm i eslint@^8 @typescript-eslint/parser@^5 eslint-plugin-import@^2 and they get a significantly faster lint run without any config changes at all?

[bradzacher]

It seems like we could potentially get more bang for the buck by creating a plugin solution that encapsulated parsing, type information, and rules.

From our POV all we care about is two things:

1. 3rd parties outside of "our plugin" can access the type information.
   • There is a significant ecosystem of plugins outside of @typescript-eslint that are type-aware, consuming the types that we provide from @typescript-eslint/parser.
   • We cannot break this usecase or else we irrevocably harm the ecosystem.
2. Type information is calculated before any files are parsed (and thus before any files are linted).
   • The type information data structures include the TS ASTs which we then convert to the TS-ESTree format.
   • Whilst technically we could parse files separately of the type information - it would ultimately just be duplicated, wasted effort that slows the lint run down (i.e. file is parsed once by TS for it to calculate types and parsed separately by us for ESLint).

Beyond that (at least at the surface level) I don't think it makes much of a difference to us how ESLint requires us to structure things.

[nzakas]

@bradzacher thanks for sharing that data, that is eye-opening. As always, numbers without context are difficult to interpret, but I strongly doubt that every download is someone who is actively developing their project. My hunch is that the ESLint 7 & 8 numbers are more representative of active development than the previous ones.

Regardless, there is always a tension between continuing to iterate on what already exists vs. creating a hard stop and cutting over to something new. The cold, hard fact is that we simply don't have the bandwidth to do both. If we continue to iterate on eslintrc, we will continue to need to support it and will continue to have compatibility problems with flat config. We'll never make progress that way.

The fact is, the way typescript-eslint works today is a direct result of limitations that the current ESLint core has. What I'm suggesting is that you start thinking not about what works today, but instead, think about what typescript-eslint could be if ESLint provides more hooks and knobs to play with. This is the time to rethink things and get us that feedback.

This proposal, as-is, is still leaning heavily on all of the existing ESLint constraints and that's not where we want to go in the future. In my mind, I think there's a lot of leverage to thinking about TypeScript support as the driving factor in the ESLint rewrite and I'd like to encourage you to think along those lines. Let's figure out what a perfect world free of the current ESLint constraints looks like and then figure out a path from here to there.

[JoshuaKGoldberg]

@nzakas following #102 (comment), I've reduced the scope of the RFC slightly to a "persistent" | "single" | "single-with-fixer" mode to parsers, not the full options object. What do you think about this process.env option as a further dev+maintenance cost reduction?

I personally wouldn't choose it for anything long-lived, but I think it resolves the "sorry, that's not supported" worry - as it's a piece of information the next generation of parser-equivalents will need to be provided no matter what.

Failing these alternatives, do you have any suggestions for something that might resolve your concerns sooner than the complete rewrite? Or failing that, other alternatives that don't, to help me try to think through them? 🙂

[JoshuaKGoldberg]

Maybe... "single-with-fix"? I'm not super positive about this new name either
way... Does anybody have a suggestion for something less verbose?

[JoshuaKGoldberg]

By the way, we hear you loud and clear that there is minimal-at-best budget on dev+maintenance for the legacy systems in ESLint, and the best path forward is to work on creating the new/rewrite system in a way that supports the needed logic. 100%. We're actively trying to help how we can there too - and if there's anything more we can be doing, please let us know!

What I'm trying to do in this RFC is see if we can get in the feature that'd give a nice performance boost to users today at a cheap enough dev+maintenance cost to justify it. We can already demonstrate an estimated >=10% performance improvement for an estimated >=8-10% of ESLint users today in just one typescript-eslint change alone. So we're very motivated to try to find a compromise here! For example, the process.env suggestion being a bandaid fix makes me happy because it's not many lines of code, even if it is an interim bandaid solution at best.

[nzakas]

My hesitation here is that I think this needs a lot more deep thought as part of a larger rewrite than we are giving it. Again, what if we add something now that we figure it actually wrong or doesn't work with the new rewrite? Because we tend to favor compatibility, we'll be stuck with it for a while as we transition people off of it. And there's also the slow drip problem, where once we add some notion into ESLint somewhere then people are going to want it surfaced in other areas and we'll have to deal with the fallout of that. Features are like viruses in that way. (It's possible others on the team feel differently, so I'll let them speak to that.)

At this point, the most I would want to do was somehow indicate the session is not persistent. I wouldn't want to mess with whether or not it was in fix mode, as it doesn't seem that there's a lot of incremental benefit to that.

But, I'm still not convinced that the gains are worth rushing or shoehorning in a half-solution vs. doing a deep dive as part of the rewrite and getting it correct. I'd like some direction from the rest of the team on this.

[nzakas]

@mdjermanovic still waiting for your feedback on this.

[mdjermanovic]

I agree with @nzakas. Given the upcoming rewrite, expanding APIs in a likely future-incompatible way doesn't seem like the right direction at this point.

@nzakas what do you think about the process.env alternative?

[mdjermanovic]

Instead of calculating context in a certain way and thereby assuming what's important for consumers, I'd prefer that we set simple, more raw data about the execution context.

For example:

// somewhere in ESLint CLI modules

process.env.ESLINT_CLI_RUN = "true";

if (fix) {
    process.env.ESLINT_CLI_FIX= "true";
}

[nzakas]

Even with process.env, I feel like we are doing something hacky that we won't want to do in the future, creating more tech debt to clean up out of the ecosystem.

Ultimately, I think we want an API where consumers must declare if their session is persistent or not right inside the API and not look for outside resources to try and figure that out.

[bradzacher]

I totally understand and respect the reluctance to build any new systems into the old codebase that's going to go away in the future.

The thing is though that we know these are issues right now, and we have clear pathway to being able to improve the user experience right now (we have good data that shows how much we can improve the single-run speed if we can clearly determine it is a single run).

It just seems wrong to be to block this improvement just because "the next version will fix it" namely because there's no clear timeline on when the next version will be.
I could understand if we had a clear picture of when - like months, quarters or even halves away - but realistically it could be over a year until the next version is ready for users.

Is there some way we can move forward on an API here and earmark it as a temporary thing we won't support in the new version? Or is there some way we can design this to work with the new version in mind? Or perhaps we just ensure the new version uses a new exported parser function name so it's not tied to any decision we make here?

[nzakas]

We talked about this again today and have decided not to move forward. While I understand the rationale here from previous discussions (and the phone call), we don't feel comfortable with rushing forward with a solution that might not be the final one. We know there is some pain right now, but it's pain we've been living with for a while so it doesn't seem like shoehorning something in immediately to solve it temporarily is the right approach.

We will be figuring out a holistic strategy around sessions going forward, and we'll be sure to loop you into those conversations.