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

Conversation

mysticatea
Copy link
Member

@mysticatea mysticatea commented Sep 28, 2019

Summary

This RFC changes the methods of CLIEngine class as returning Promise and adds CLIEngineSync class for soft-migration.

This RFC adds a new class LinterShell that provides asynchronous API and deprecates CLIEngine.

Related Issues

@mysticatea mysticatea added the Initial Commenting This RFC is in the initial feedback stage label Sep 28, 2019
Copy link
Member

@ilyavolodin ilyavolodin left a comment

Choose a reason for hiding this comment

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

This would be a major breaking change, that will affect all integrations. I don't think it's justified at all. I think it would be better to do it the other way around. Introduce CLIEngineAsync with async properties, and keep existing CLIEngine synchronous. In that case, it will be a non-breaking change.

@mysticatea
Copy link
Member Author

There is the reason I didn't write reverse.

  • Dynamic loading of ES modules requires asynchronous API. Migrating to asynchronous API opens up doors to write plugins/configs with ES modules.
  • 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.)

Multiple functionalities require async API and sync API looks closing doors for the future.

@ilyavolodin
Copy link
Member

ilyavolodin commented Sep 28, 2019

The opposite is also true:
Async APIs make tracing and debugging significantly harder, can lead to a lot more unforeseen errors and, in some cases, significantly raise complexity in submitting small fixes.
Sorry, I do not find your first bullet point compelling at all. Being able to write plugins/configs with ES modules is nice, but by no means critical from my perspective, especially given that ES modules in Node are still behind the flag, and require other concessions like using mjs file extensions.

@mysticatea
Copy link
Member Author

In fact, don't you think you want to add the parallel linting feature? The parallel linting requires async API and your statement sounds to strongly oppose it.

@ilyavolodin
Copy link
Member

No, you are misunderstanding me. I'm not against introduction of async API. I just don't think it should be default API that we expose, as it would be a huge breaking change, and I don't see enough justification for it. I'm perfectly fine with leaving default API as sync, and introducing a new async API as optional. Parallel linting doesn't require async API, it would make things a lot simple, but it can be achieved without async API.

@mysticatea
Copy link
Member Author

Thank you for clarification.

How about adding a new class and deprecating CLIEngine?
Maybe the name of the new class is ESLint and it has async API.

I remember the name of CLIEngine causes large confusion to the community. People try Linter class at first, then they notice it doesn't work as expected. We have a lot of issues that say "please use CLIEngine instead."

Parallel linting doesn't require async API, it would make things a lot simple, but it can be achieved without async API.

Out of curiosity, how can we implement it? As far as I know, worker_threads module has the event-based API. I'm not sure how we use it in sync API.

@ilyavolodin
Copy link
Member

I think deprecating CLIEngine is a better path forward. It would still be a breaking change, but as long as we keep CLIEngine around, it will not break existing integrations.
In terms of how to implement parallel linting, to clarify, you do need some functions to be converted to async, basically everything that comes before forking needs to be async, but everything inside the forks can stay sync. Also, just because it has to be async, doesn't mean we can't do it side-by-side with sync functions, and keep sync functions as default.

@mysticatea
Copy link
Member Author

I got it. Thank you for your feedback. I'm rewriting this RFC.

@mysticatea
Copy link
Member Author

I have updated this RFC.

Copy link
Member

@ilyavolodin ilyavolodin left a comment

Choose a reason for hiding this comment

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

I think this approach is much better direction then before. But I still wonder if alternative that you mention would be even easier approach?

designs/2019-move-to-async-api/README.md Outdated Show resolved Hide resolved
designs/2019-move-to-async-api/README.md Show resolved Hide resolved
designs/2019-move-to-async-api/README.md Outdated Show resolved Hide resolved
@mysticatea
Copy link
Member Author

I updated this RFC as changing getFormatter() to use streams.

@mysticatea
Copy link
Member Author

I updated this RFC as reconsider some methods with Async Iteration.

  • Now the executeOnFiles() method returns AsyncIterator<LintResult> instead of Promise<LintResult[]>. Therefore, in the future, we should be able to support formatters that print progress state without more breaking changes.
  • The getFormatter() method returns a promise object that will be fulfilled with a formatter. Now the formatter type is (results: AsyncIterator<LintResult>) => AsyncIterator<string>. The adapter for the current formatter spec is this code. In the future, we should be able to support the formatter that does streaming output without more breaking changes, as updating the adapter.
  • I have added more description, especially, what the API change gives us.

Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

Looks pretty good so far. I'll continue to follow this discussion.

- change executeOnFiles method to support simple await expression
- change executeOnText method to align the return type to executeOnFiles method
- add ESLint.compareResultsByFilePath static method
@mysticatea
Copy link
Member Author

I updated this RFC.

  • Now we can use the executeOnFiles() method along with both async expressions and for-await-of statements. It would be convenient in the cases that users want to get the lint results at once.
  • Now the two methods executeOnFiles() and executeOnText() have the same return type. This inherits CLIEngine's manner.
  • I added ESLint.compareResultsByFilePath() static method in order to sort the lint results in a stable order because I thought it's a common use-case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Final Commenting This RFC is in the final week of commenting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants