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: ESLint Class Replacing CLIEngine #40

Merged
merged 47 commits into from
Jan 6, 2020
Merged
Changes from 12 commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
b1840be
New: Moving to Asynchronous API
mysticatea Sep 28, 2019
6135383
add PR link
mysticatea Sep 28, 2019
0af99b0
fix to not change existing methods
mysticatea Sep 28, 2019
d9444ef
change getFormatter
mysticatea Oct 1, 2019
e5cc07c
fix getFormatter
mysticatea Oct 2, 2019
be5878c
update
mysticatea Oct 3, 2019
628e262
update Backwards Compatibility Analysis section
mysticatea Oct 3, 2019
cade0bd
add an example
mysticatea Oct 3, 2019
c99ae56
trivial fix
mysticatea Oct 3, 2019
730cb58
Add a note about streams
mysticatea Oct 3, 2019
2ab427f
trivial fix
mysticatea Oct 3, 2019
ed35022
update getErrorResults since it's sandwiched
mysticatea Oct 3, 2019
13354ce
add fail-fast note
mysticatea Oct 4, 2019
2aa4a5e
update
mysticatea Oct 8, 2019
8892dc7
refactor and add about usedDeprecatedRules
mysticatea Oct 13, 2019
7a94b03
fix typo
mysticatea Oct 13, 2019
7b5e401
fix typo
mysticatea Oct 14, 2019
2981228
add about aborting
mysticatea Oct 14, 2019
72e69af
add constructor and remove addPlugin
mysticatea Oct 14, 2019
86689ec
Update motivation section
mysticatea Oct 15, 2019
a76d6c0
Update motivation section
mysticatea Oct 15, 2019
face206
Update design overview
mysticatea Oct 15, 2019
65b67f5
Update constructor description
mysticatea Oct 15, 2019
cedb6e6
Update fail-fast on parallel calling section
mysticatea Oct 15, 2019
8f58df9
Update designs/2019-move-to-async-api/README.md
mysticatea Oct 15, 2019
227fe54
Update designs/2019-move-to-async-api/README.md
mysticatea Oct 15, 2019
4c5cdff
update RFC for more clarification and...
mysticatea Oct 17, 2019
00be95b
add a related discussion
mysticatea Oct 17, 2019
756e929
add about cache when abort
mysticatea Oct 21, 2019
975fa02
add TOC of methods
mysticatea Oct 21, 2019
f22894b
update about disallow execution in parallel
mysticatea Oct 21, 2019
d45e86e
rewrite "Alternatives" section
mysticatea Oct 21, 2019
c5bdf22
revert about errorsOnly and...
mysticatea Oct 21, 2019
6ecf034
small fix
mysticatea Oct 21, 2019
98243b8
remove collectResults method
mysticatea Nov 14, 2019
13650ff
remove getRules()
mysticatea Nov 14, 2019
a860d00
update the sentence about ES modules
mysticatea Nov 14, 2019
eeac990
rename for lintFiles() and lintText()
mysticatea Nov 14, 2019
6a36a14
replace pluginImplementations option
mysticatea Nov 14, 2019
ab978fa
update about cache handling
mysticatea Nov 14, 2019
920c881
rename the new class
mysticatea Nov 22, 2019
8547265
remove about to print progress
mysticatea Nov 30, 2019
c24f337
Update designs/2019-move-to-async-api/README.md
mysticatea Dec 5, 2019
9d4affc
Update designs/2019-move-to-async-api/README.md
mysticatea Dec 8, 2019
5032c3c
Update designs/2019-move-to-async-api/README.md
mysticatea Dec 8, 2019
671cf6a
remove about printing progress
mysticatea Dec 8, 2019
ef4d198
ESLint class again
mysticatea Dec 8, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
247 changes: 247 additions & 0 deletions designs/2019-move-to-async-api/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,247 @@
- Start Date: 2019-09-28
- RFC PR: https://github.com/eslint/rfcs/pull/40
- Authors: Toru Nagashima ([@mysticatea](https://github.com/mysticatea))

# Moving to Asynchronous API
mysticatea marked this conversation as resolved.
Show resolved Hide resolved

## Summary

This RFC adds a new class `ESLint` that has asynchronous API and deprecates `CLIEngine`.

## Motivation

- Dynamic `import()` has arrived at Stage 4. The dynamic loading of ES modules requires asynchronous API. Migrating to asynchronous API opens up doors to write plugins/configs with ES modules.
mysticatea marked this conversation as resolved.
Show resolved Hide resolved
- Linting in parallel requires asynchronous API. We can improve linting logic to run it in parallel. And we can improve config loading logic and file enumeration logic to run it in parallel. (E.g., load `extends` list, traverse child directories.)

Because the migration of public API needs long time, we should start to migrate our APIs earlier.

And the name of `CLIEngine`, our primary API, causes confusion to the community. People try `Linter` class at first because the name sounds the main class of ESLint, then they notice it doesn't work as expected. We have a lot of issues that say "please use `CLIEngine` instead."
The name of new class, `ESLint`, is our primary API clearly.

## Detailed Design

### ■ Add new `ESLint` class

This RFC adds a new class `ESLint`. It has almost the same methods as `CLIEngine`, but the return value of some methods are different.

So, for now, `ESLint` class will be a tiny wrapper of `CLIEngine` that modifies the type of the return values.
mysticatea marked this conversation as resolved.
Show resolved Hide resolved

#### § The `executeOnFiles()` method

This method returns a `AsyncIterator<LintResult>` object iterates the lint result of each file in random order.

<details>
<summary>A rough sketch of the `executeOnFiles()` method.</summary>

```js
class ESLint {
async *executeOnFiles(patterns) {
// Verify files and push the results.
for (const result of this.cliEngine.executeOnFiles(patterns).results) {
yield result
}
}
}
```

</details>

<details>
<summary>Example: Show the results step by step.</summary>

```js
const { ESLint } = require("eslint")
const eslint = new ESLint()

for await (const result of eslint.executeOnFiles(patterns)) {
print(result)
}
```

</details>

<details>
<summary>Example: Show the results in the stable order.</summary>

```js
const { ESLint } = require("eslint")
const eslint = new ESLint()
const results = []

for await (const result of eslint.executeOnFiles(patterns)) {
results.push(result)
}

results.sort(byFilePath)
print(results)
```

</details>

Once the `executeOnFiles()` method got this change, we can support "linting in parallel", streaming, and plugins/configs which are ES modules in the future.

#### § The `getFormatter()` method

This method returns a `Promise<Formatter>`. The `Formatter` type is a function `(results: AsyncIterator<LintResult>) => AsyncIterator<string>`. It receives lint results then outputs the formatted text.

This means the `getFormatter()` method wraps the current formatter to align the interface.

<details>
<summary>A rough sketch of the `getFormatter()` method.</summary>

```js
class ESLint {
async getFormatter(name) {
const format = this.cliEngine.getFormatter(name)

// Return the wrapper.
return async function* formatter(resultIterator) {
// Collect the results.
const results = []
for await (const result of resultIterator) {
results.push(result)
}
results.sort(byFilePath)

// Make `rulesMeta`.
const rules = this.cliEngine.getRules()
const rulesMeta = getRulesMeta(rules)

// Format the results with the formatter of the current spec.
yield format(results, { rulesMeta })
}
}
}
```

</details>

<details>
<summary>Example: Use the formatter.</summary>

```js
const { ESLint } = require("eslint")
const eslint = new ESLint()
const formatter = eslint.getFormatter("stylish")

// Verify files
const results = eslint.executeOnFiles(patterns)
// Format and write the results
for await (const textPiece of formatter(results)) {
process.stdout.write(textPiece)
}
```

</details>

Once the `getFormatter()` method got this change, we can update the specification of custom formatters without breakage in the future to support streaming.

Choose a reason for hiding this comment

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

See my previous comment. Should probably be more fleshed out the intention of why this is an async generator and how that would work right here and now and how that would work eventually in the future.

It should probably also be added some reasoning on why the formatters are okay with being pure async generators while executeOnFiles has been made a cross-product of async generators and promises. Should the two be consistent in that regard?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should the two be consistent in that regard?

Because formatters are used to output results to stdout or a file, and both are streams, so I didn't think that it should be Thenable.


#### § The `getErrorResults()` method

As related to the above, this method now receives an `AsyncIterator<LintResult>` object and returns an `AsyncIterator<LintResult>` object. Because this method is sandwiched between `executeOnFiles()` and formatters.

<details>
<summary>A rough sketch of the `getErrorResults()` static method.</summary>

```js
class ESLint {
static async *getErrorResults(results) {
for await (const result of results) {
const messages = result.messages.filter(m => m.severity === 2)

if (messages.length === result.messages.length) {
yield result
}
yield {
...result,
messages,
warningCount: 0,
fixableWarningCount: 0,
}
}
}
}
```

</details>

<details>
<summary>Example: Use `getErrorResults()`.</summary>

```js
const { ESLint } = require("eslint")
const eslint = new ESLint()
const formatter = eslint.getFormatter("stylish")

// Verify files
let results = eslint.executeOnFiles(patterns)
// Filter the results if needed
if (process.argv.includes("--quiet")) {
results = ESLint.getErrorResults(results)
}
// Format and write the results
for await (const textPiece of formatter(results)) {
process.stdout.write(textPiece)
}
```

</details>

#### § The other methods

The following methods return `Promise` which gets fulfilled with each result.

- `executeOnText()`
- `getConfigForFile()`
mysticatea marked this conversation as resolved.
Show resolved Hide resolved
- `isPathIgnored()`
- `outputFixes()`

Once the former three methods got this change, we can support plugins/configs that are ES modules without breakage in the future. And once the `outputFixes()` method got this change, we can write many files more efficiently in the future.

The following methods are as-is because those don't touch both file system and module system.

- `addPlugin()`
- `getRules()`

The following methods are removed because those don't fit the current API.

- `resolveFileGlobPatterns()` ... ESLint doesn't use this logic since `v6.0.0`, but it has stayed there for backward compatibility. Once [RFC 20](https://github.com/eslint/rfcs/tree/master/designs/2019-additional-lint-targets) is implemented, what ESLint iterates and what the glob of this method iterates will be different, then it will confuse users. This is good timing to remove the legacy.

### ■ Deprecate `CLIEngine` class

This RFC soft-deprecates `CLIEngine` class.

Choose a reason for hiding this comment

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

May I suggest adding something similar to:

- Adds `disallowWorkerThreads` option to plugins. Defaults to `false`.

Like:

- Adds `requiresAsyncUse` option to plugins. Defaults to `false`.

That way any plugin which wants to move to fully use the new async features can do so without risking incompatibilities with the sync flow.

If that property is set on a plugin, then the sync flow of CLIEngine should error when encountering it and tell the user about the incompatibility.

This would help the ecosystem in regards to the deprecation and the move towards an async flow.

Copy link
Member Author

@mysticatea mysticatea Oct 15, 2019

Choose a reason for hiding this comment

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

I'm not sure if there are incompatibility things about this. We don't provide any asynchronous API to plugins. On the other hand, running in parallel can break plugins that use OS resources exclusively (e.g., cache file).

In the future, if we support asynchronous stuff for plugins (e.g., ES modules. It requires import() to load the plugins), CLIEngine fails to load those. I don't think that we need additional flags.


Because it's tough to maintain two versions (sync and async) of implementation. The two are almost copy-pasted stuff, but hard to share the code. Therefore, this RFC deprecates the sync version to improve our code with the way which needs asynchronous behavior in the future. For example, `CLIEngine` cannot support parallel linting, plugins/configs as ES modules, etc...

### ■ Out of scope

- Not change API for rules. This RFC doesn't change APIs that rule implementation uses. We may be able to support asynchronous stuff in rules in the future, but it's out of this RFC's scope.
- Not change internal logics. This RFC just adds the public interface that is asynchronous. It would be a wrapper of `CLIEngine` for now.

## Documentation

- The "[Node.js API](https://eslint.org/docs/developer-guide/nodejs-api)" page should describe the new public API and deprecation of `CLIEngine` class.

## Drawbacks

People that use `CLIEngine` have to update their application with the new API. It will need hard work.

## Backwards Compatibility Analysis

This is a breaking change.

Deprecating `CLIEngine` is a drastic change. But people can continue to use `CLIEngine` as-is until we decide to remove it.

The new API depends on [Asynchronous Iteration](https://github.com/tc39/proposal-async-iteration) syntax. Node.js supports the syntax since `10.0.0`, so we have to drop Node.js `8.x`. Because the `8.x` will be EOL in December 2019 (two months later!), we can work on this soon.

## Alternatives

- Adding `engine.executeAsyncOnFiles()`-like methods and we maintain it along with the existing synchronous API. But as what I wrote in the "[Deprecate `CLIEngine` class](#deprecate-cliengine-class)" section, it would be tough.
mysticatea marked this conversation as resolved.
Show resolved Hide resolved
- Using [Streams](https://nodejs.org/dist/latest-v12.x/docs/api/stream.html) instead of [Asynchronous Iteration](https://github.com/tc39/proposal-async-iteration). We can introduce `ESLint` class in a minor release if we used Streams. But because Node.js 8 will be EOL two months later, we should be able to use Asynchronous Iteration soon. Iterator protocol is smaller spec than streams, and it's easy to use.

## Related Discussions

- https://github.com/eslint/eslint/issues/3565 - Lint multiple files in parallel
- https://github.com/eslint/eslint/issues/12319 - `ERR_REQUIRE_ESM` when requiring `.eslintrc.js`
- https://github.com/eslint/rfcs/pull/4 - New: added draft for async/parallel proposal
- https://github.com/eslint/rfcs/pull/11 - New: Lint files in parallel