8000 feat!: Support async parsing by mdjermanovic · Pull Request #87 · eslint/rfcs · GitHub
[go: up one dir, main page]

Skip to content

feat!: Support async parsing #87

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

Closed
wants to merge 2 commits into from
Closed

feat!: Support async parsing #87

wants to merge 2 commits into from

Conversation

mdjermanovic
Copy link
Member

Summary

This RFC proposes adding support for asynchronous parser API.

Related Issues

@mdjermanovic mdjermanovic added feature breaking change This RFC contains breaking changes labels Mar 18, 2022
@mdjermanovic mdjermanovic added Initial Commenting This RFC is in the initial feedback stage and removed breaking change This RFC contains breaking changes labels Mar 18, 2022
## Open Questions

* Do we want to explore parallel parsing-linting as described in https://github.com/eslint/eslint/discussions/14733 in this RFC or separately?
* Assuming that we plan to implement both `async parse()` support and Flat Config for v9.0.0, can we split `FlatLinter` out of the `Linter` now? The work on `async parse()` overlaps a lot with the work on flat config. With the current dual-mode `Linter`, it doesn't seem feasible to start implementing support for `async parse()` before the final v9.0.0 flat config changes are merged. If we split out `FlatLinter`, most of the work could be finished much earlier and we could have a working support for async parsers in the "flat" mode (the mode that uses `FlatESLint` class).
Copy link
Member

Choose a reason for hiding this comment

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

This is an idea worth exploring. I’m a bit concerned about the effects on the ecosystem of changing formerly sync methods to be async. We don’t really know all the places that Linter is used, so such a change could break a lot of places we just aren’t aware of yet. (Maybe @bmish could modify his GitHub search he used for rule formats to see where Linter is being used?)

Tying the new async support to flat config could be a nice transitional phase to smooth over this change. There would be the open question of how long to keep the old Linter class around in this case.

Note that we ultimately don’t want the word “flat” in any interface names as flat config will be the only config eventually.

Copy link

Choose a reason for hiding this comment

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

My guess would be that it makes it prohibitively difficult to maintain an eslint plugin that’s compatible with both 8 and 9 (assuming 9 is what adds this).

Copy link
Member Author

Choose a reason for hiding this comment

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

@ljharb do you know of any eslint plugins that use the Linter class? That sounds like a very unusual use case to me.

Copy link

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Keep in mind that the ecosystem is more than just plugins. We need to consider other tools that may wrap ESLint in some way, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

are all i found in a quick search.

Linter#getRules and Linter#defineRule wouldn't be affected by this change (they will be removed by eslint/eslint#13481 though, but that's off-topic for this RFC).

I should have been more precise, the question is which plugins and tools use Linter#verify or Linter#verifyAndFix, as those are the only public methods that will be changed by this RFC. The change would be especially problematic for plugins because rules and processors cannot be async.

Copy link
Member

Choose a reason for hiding this comment

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

In the first several pages of a GitHub search for linter.verify() in code, all of the results are in test files. There are a few instances of linter.verifyAndFix() outside of tests.

## Alternatives

### Alternatives to `async parse()` / `async parseForESLint()` 8000

Copy link
Member

Choose a reason for hiding this comment

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

At the moment I have function configs disabled in flat config due to the complexity of getting it working.

Copy link
Member

Choose a reason for hiding this comment

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

Another option: we could actually do the async loading inside of the ESLint class as part of preprocessing and then stick the loaded module directly into the config that is passed to Linter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Another option: we could actually do the async loading inside of the ESLint class as part of preprocessing and then stick the loaded module directly into the config that is passed to Linter.

I've added this alternative and another one with top-level await.

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 excited to see your thoughts about how to address this. I’m a bit concerned with the big cutover from sync to async methods as I’m not sure about the ecosystem impact, but I think there are enough alternatives that there’s a path forward.

@mdjermanovic mdjermanovic changed the title feat!: Support async parse() / async parseForESLint() feat!: Support async parsing Mar 18, 2022

## Motivation

Per the [original issue](https://github.com/eslint/eslint/issues/15475), this feature will allow for the use of ESLint with custom parsers written in languages that compile to Wasm or other binary formats.
Copy link

Choose a reason for hiding this comment

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

This motivation is speculative since there is no such asynchronous parser for now. SWC is a candidate but it has performance problem passing AST to JS. swc-project/swc#2175

Copy link
Member

Choose a reason for hiding this comment

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

I’d say anticipatory rather than speculative. People won’t write parsers that require async loading until ESLint supports async loading. We want to allow for the possibility sooner rather than later.

Copy link

Choose a reason for hiding this comment

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

the import plugin would likely benefit massively from an async parser, that can make parsing multiple files concurrent across the whole codebase.


* In a preprocessing step that we could add to the `ESLint` class.
* In an async function inside `eslint.config.js`, if the new flat config system supports [async function configs](https://github.com/eslint/rfcs/tree/main/designs/2019-config-simplification#can-a-config-be-an-async-function).
* With top-level `await` in `eslint.config.js`, if the new flat config system supports ESM configs.
Copy link
Member

Choose a reason for hiding this comment

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

ESM configs are fully supported. 👍

@nzakas
Copy link
Member
nzakas commented Apr 15, 2022

@btmills thoughts on this?

Copy link
Member
@btmills btmills left a comment

Choose a reason for hiding this comment

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

D'oh, turns out it helps if you actually click submit on the review.


Most changes are straightforward. Functions that operate on results from other async functions will become `async` functions themselves, and they'll `await` those results. Functions that just `return` results from other async functions don't need to `await` those results, but we'll nevertheless convert them to `async` functions to ensure consistent signature.

`_verifyWithProcessor` will lint code blocks one by one, as before this change. The linting of the second code block will start after the linting of the first code block has finished, and so on.
Copy link
Member

Choose a reason for hiding this comment

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

Is this a note to simplify initial implementation and something that we could change later, or is this somehow a constraint of the design?


Breaking changes for plugins are only related to `RuleTester`:

* `RuleTester` will require a testing framework or custom `it` handler.
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a deprecation warning when itDefaultHandler is used to encourage people to customize it prior to v9.0.0?

@nzakas
Copy link
Member
nzakas commented Apr 22, 2022

The more I’ve thought about it, the more I don’t believe that we can do a cutover from sync methods to async methods. We just don’t know all of the places that the Linter methods are used assuming they are sync, and I keep finding more examples. For instance, the JavaScript editor for CodeMirror allows you to pass in a Linter instance:
https://www.npmjs.com/package/@codemirror/lang-javascript#user-content-eslint

So, I think the only suitable path forward likely involves leaving the sync methods as-is and either creating async methods alongside or else creating a new class that only has async methods.

@btmills
Copy link
Member
btmills commented May 7, 2022

If we shouldn't expect the ecosystem to implement an async API, is it worth having async equivalents at all? We know of two use cases for async parsing: using wasm parsers and allowing eslint-plugin-import to parallelize parsing other files. I'd like to have async parsing, but I'm wondering whether we'd ever see the benefits if we did it as Linter#verifyAsync() or AsyncLinter#verify().

@bradzacher
Copy link
bradzacher commented May 7, 2022

@typescript-eslint would also benefit from this by being able to parallelise the "project"-based (type-aware) parsing using TS.

I've already done some investigation into this by imementing a custom CLI and it showed some tangible perf wins (typescript-eslint/typescript-eslint#4359)

@btmills
Copy link
Member
btmills commented May 7, 2022

For wasm parsers, there are other ways mentioned in this RFC that we could do the async initialization. If we took that alternative and resurrected the discussion started in eslint/eslint#14139, would we need async parsing anymore?

@bradzacher
Copy link

For @typescript-eslint - the goal would be parallel initialisation into multiple worker threads (one per TS project config). We would need async parsing so that we could communicate with the worker threads after the fact.
Unless I'm ignorant of a synchronous method of communicating to worker threads in nodejs?

@nzakas
Copy link
Member
nzakas commented May 11, 2022

If we shouldn't expect the ecosystem to implement an async API, is it worth having async equivalents at all?

I don’t think this is necessarily the right question. My point is that we can’t just replace sync methods with async ones because that will make it difficult for folks to migrate, as some circumstances currently don’t allow async calls (like inside of rules). That’s a harsh cutover without any migration path.

Supporting both would give us a migration path while also allowing experimentation. It seems that people expect to be able to use async functions in places where regular functions are used. We’ve had numerous requests for both async parsing and async rules, and I think there’s good argument to do async all the way down to bring ESLint up to date with the rest of the JS ecosystem.

So while we can punt on this for now, I fully expect that down the line we will have more requests and we will eventually need to solve it.

@btmills
Copy link
Member
btmills commented May 19, 2022

While I’m not thrilled about maintaining both sync and async implementations, you’ve persuaded me that it’s the best route toward where we’d like to end up long term.

Between async methods or an all-async class, right now I’d lean toward async methods to learn about the use cases. Eventually we’ll probably want an all-async class (perhaps using parallel linting as a carrot), but I doubt we know enough today to design that class’s complete API. If we get these methods right, we could include compatible versions in a new class in the future.

@nzakas
Copy link
Member
nzakas commented Mar 8, 2023

Closing, as we will address this as part of the larger rewrite.

@nzakas nzakas closed this Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking feature Initial Commenting This RFC is in the initial feedback stage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
0