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: Add public APIs for config-arrays and file-infos #52

Closed
wants to merge 2 commits into from
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
85 changes: 85 additions & 0 deletions designs/2019-public-config-array-api/README.md
@@ -0,0 +1,85 @@
- Start Date: 2019-12-09
- RFC PR: <!-- leave this empty, to be filled in later -->
manuth marked this conversation as resolved.
Show resolved Hide resolved
- Authors: @manuth

# Public `ConfigArray` API

## Summary

Allow 3rd-party developers to determine which configuration-files are considered by `eslint`.

## Motivation

In some cases people might want to automatically trigger a new linting process as soon as the configuration of eslint has changed.
This is a key feature for language servers which provide warnings and fixes to IDEs.

Therefore the language-server needs to know which config-files are considered by `eslint`.
Comment on lines +13 to +16
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for your proposal.

I agree that exposing ConfigArray is worthful, but I feel that this goal and this proposal is different a bit because ConfigArray doesn't have enough information for that. There are related files to configurations:

  1. Config files
  2. Shareable config files -- it's needed to re-run when we upgraded shareable configs.
  3. Parser files -- it's needed to re-run when we upgraded parsers.
  4. Plugin files -- it's needed to re-run when we upgraded plugins.
  5. Rule files that --rulesdir option specified -- it's needed to re-run when we modified local rules.
  6. .eslintignore -- it's needed to re-run when we modified .eslintignore.
  7. The files that are imported by require("source") in the above files. -- it's needed to re-run if config files or other files imported other files and the imported files are modified.
  8. The files of ESLint -- it's needed to re-run when we upgraded ESLint. This is a common case because ESLint is regularly updated every month.

The filePath property of ConfigArray contains 1., 2., and 6. But if we approve #9, it will contain only $CWD/eslint.config.js because all other files become "the files that are imported by require("source")." Therefore, I guess that the filePath property of ConfigArray will be not useful for this purpose.

If you want to get only 1., actually, it may be hard to distinguish if the filePath property of ConfigArray is 1. or not. Maybe true if the path is $CWD/** but not node_modules/**.


It's hard to know the files that are imported by require("source").

And Node.js has the cache for imported files. If we don't clear the cache, for example, when we updated plugin versions then the language server may use old plugin code partially and raise cryptic errors. Currently, we can clear the cache with require.cache, but if we started to use import("source") instead (it's required in order to support ES modules for plugins/configs) in the near future, we will lose the capability to clear the cache. Therefore, I think hard to re-run linting without restarting the Node.js process.

Copy link
Author

Choose a reason for hiding this comment

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

Humh...
I totally get your point - adding such a method to the public API, in this case, wouldn't add the desired ability of knowing which files to watch.

import {} from "source";, import "source"; and import var = require("source");, btw, affect require.cache as well so clearing or checking the cache would work out pretty well also for ES modules.

Anyway to reach the goal I'm after you'd have to import the plugins and settings and check require.cache for changes, right?

Sounds like this would require another separate method other than getConfigArrayForFile?

Copy link
Member

Choose a reason for hiding this comment

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

btw, affect require.cache as well so clearing or checking the cache would work out pretty well also for ES modules.

It's only if the "source" was CJS. If the "source" is ESM, Node.js caches it only in the internal cache and we cannot access it. The require.cache doesn't include imported ESM files.

Anyway to reach the goal I'm after you'd have to import the plugins and settings and check require.cache for changes, right?

Currently, ESLint doesn't support ESM, so it may work. However, after we supported ESM, it's going to not work properly.

Sounds like this would require another separate method other than getConfigArrayForFile?

Yes. I think getConfigArrayForFile is not useful for this purpose.

Copy link
Author

@manuth manuth Dec 11, 2019

Choose a reason for hiding this comment

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

Alright - so cleaning the cache is of no use as it will stop working at some point anyway...?

Forking a new process might be of no use, too, as this would draw me unable to get the result from eslint... Or is there some way to pass JavaScript-objects from/to forked processes or any other alternative?

Yes. I think getConfigArrayForFile is not useful for this purpose.

Fine - shall we cover both the getConfigArrayForFile and the language-server stuff in this RFC?


## Detailed Design

A method called `getConfigArrayForFile()` shall be added to the `CLIEngine` class.
By providing such a feature people can easily watch the correct files for changes as soon as the configuration changes.

***Example:***
```js
import fs = require("fs");

let engine = new CLIEngine({});
let file = "./App/src/index.ts";
let configArray = engine.getConfigArrayForFile(file);

for (let config of configArray) {
if (config.filePath.length > 0) {
fs.watchFile(
conig.filePath,
() =>
{
lint();
});
}
}

function lint() {
engine.executeOnFiles([file]);
}

lint();
```

## Documentation

I think it might be a good idea to blog about this new feature as this is a key feature for writing language servers for IDEs.
That way the integration of `eslint` in all sorts of text-editors can be improved to its best.

## Drawbacks

The only downside I can think of is that on every change to the `ConfigArray` or `CascadingConfigArray` API either a new major version has to be released or a separate implementation of said classes has to be added for the public API.

## Backwards Compatibility Analysis

Adding this feature provides full backwards compatibility.

## Alternatives

Add a method which only returns filepaths and a value indicating whether said filepath is `root`.
That way the drawbacks could be fixed but the developers don't have the benefit to further analyze the config-array.

A similar method already exists: The `CLIEngine#getConfigForFile`-method. Though this method does not indicate which files are considered by `eslint` which draws developers of language-servers unable to hot-reload `eslint` as soon as a config-file changes.

## Open Questions

- Is it reasonable to return the whole config-array or should such a method only return the `filePath`s?
manuth marked this conversation as resolved.
Show resolved Hide resolved
- Does it make sense to also return the `root`-directory in order to let language-servers run `eslint` with `root` as its `cwd`?
manuth marked this conversation as resolved.
Show resolved Hide resolved
- Is there some more key information other than `filePath` which should be returned? (Such as `name`, for instance)
manuth marked this conversation as resolved.
Show resolved Hide resolved

## Help Needed

I think it should be pretty easy to implement such a feature as the only thing that is required is a simple call to `CascadingConfigArrayFactory#getConfigArrayForFile` and, if required, some data extraction/manipulation.

## Frequently Asked Questions

<!-- - No FAQs added yet -->

## Related Discussions

- https://github.com/eslint/eslint/issues/12631