Skip to content

Latest commit

 

History

History
356 lines (268 loc) · 16.1 KB

README.md

File metadata and controls

356 lines (268 loc) · 16.1 KB
Raw
  • Start Date: 2019-02-25
  • RFC PR: #13
  • Authors: Toru Nagashima (@mysticatea)

Config File Improvements

Summary

This proposal improves our configuration files. This changes the architecture of configuration files to maintain our codebase easier and make enhancing easier.

This RFC fixes two bugs I found while I make a PoC. I guess we don't want to make surprised behaviors on purpose.

  • (link) Even if unused dependencies have some errors, ESLint doesn't throw it. (fixes eslint/eslint#11396)
  • (link) The configuration of overrides in shareable configs no longer overwrites user settings in .eslintrc files. (see details)

This RFC includes four enhancements.

I made the enhancements in my PoC in order to confirm this architecture change is effective to maintain our codebase easier and make enhancing easier. Therefore, the enhancements are not required for this proposal.

However, I'd like to add the enhancements with this because those will solve some important pain of the ecosystem.

Motivation

  • The codebase about configuration files is complicated. It has made us hard to maintain and enhance the configuration system.
  • The current process determines target files at first. Next, it finds configuration files on the directory where each target file exists and that ancestors. This "files-then-config" order prevents adding some enhancements such as --ext functionality to our configuration file system.
  • Several good ideas have been born in #9 and #14. We can simplify the logic about configuration files by the ideas.

Therefore, the goal of this RFC is to simplify our codebase by some architecture changes in order to maintain our codebase easier and make enhancing easier.

Then some enhancements that the simplification gives would solve the important pains of the ecosystem.

Detailed Design

(✨ in this section means the enhancement point for additional enhancement proposals.)

Proof of Concept (implementation): https://github.com/eslint/eslint/tree/proof-of-concept/config-array-in-eslintrc

  1. It simplifies the internal structure of configuration files by an array.
  2. The config object owns loaded plugins and parsers rather than registers those to somewhere.
  3. It changes the processing order to "config-then-files" from "files-then-config".
  4. It restructures the files about CLIEngine and lookup.

1. It simplifies the internal structure of configuration files by an array.

✨ When it reads configuration files, it flattens the extends property and overrides property of the configuration. This is the following order:

  1. The loaded configurations of extends property.
  2. The implicit configuration for file extension processors of plugins property.
  3. The configuration except extends property and overrides property.
  4. The configurations of overrides property.

lib/lookup/config-array-factory.js#L635-L680 in PoC.

For some details:

  • If it cannot load a configuration of extends property, it throws an error immediately.

  • If a loaded configuration of extends property has extends property or overrides property, it flattens those recursively.

    ℹ️ User-facing change:
    The configuration of overrides in shareable configs no longer overwrites user settings in .eslintrc files. (see details)

  • If the configuration has plugins property and the plugins have file extension processors, it yields config array element for those to apply the file extension processors to matched files automatically.

    lib/lookup/config-array-factory.js#L890-L901 in PoC.

  • ✨ If a configuration of overrides property has extends property or overrides property, it throws an error.

  • For duplicated settings, a later element in the array has precedence over an earlier element in the array.

💡 Example: 
{
    "extends": ["eslint:recommended", "plugin:node/recommended"],
    "rules": { ... },
    "overrides": [
        {
            "files": ["*.ts"],
            "rules": { ... },
        }
    ]
}
is flattend to: 
[
    // extends
    {
        "name": ".eslintrc.json » eslint:recommended",
        "filePath": "node_modules/eslint/conf/eslint-recommended.js",
        "rules": { ... }
    },
    {
        "name": ".eslintrc.json » plugin:node/recommended",
        "filePath": "node_modules/eslint-plugin-node/lib/index.js",
        "env": { ... },
        "parserOptions": { ... },
        "plugins": { ... },
        "rules": { ... }
    },
    // main
    {
        "name": ".eslintrc.json",
        "filePath": ".eslintrc.json",
        "rules": { ... }
    },
    // overrides
    {
        "name": ".eslintrc.json#overrides[0]",
        "filePath": ".eslintrc.json",
        "matchFile": { "includes": ["*.ts"], "excludes": null },
        "rules": { ... }
    }
]

2. The config object owns loaded plugins and parsers rather than registers those to somewhere.

The loading logic of configuration files is complicated because it has complicated relationships between the config, plugins, parsers, and environments.

Current relationship graph

The main reason is registration. The loading logic has side-effects that register loaded plugins to the plugin manager, and plugins have side-effects that register rules and environments to other managers.

The codebase gets pretty simple by the removal of the registration. Instead, the internal structure of configuration owns loaded plugins and parsers ✨.

New relationship graph

Surprisingly, now the return value of ConfigArrayFactory.loadFile(filePath) has all needed information to check files. Previously, we also needed information that was registered somewhere.

💡 Example: 
[
    {
        "name": ".eslintrc.json » plugin:@typescript-eslint/recommended",
        "filePath": "node_modules/@typescript-eslint/eslint-plugin/dist/index.js",
        // Config array element owns the loaded parser.
        "parser": {
            "definition": { ... }, // the parser implementation.
            "id": "@typescript-eslint/parser",
            "filePath": "node_modules/@typescript-eslint/parser/dist/index.js",
            "importerPath": "node_modules/@typescript-eslint/eslint-plugin/dist/index.js"
        },
        "parserOptions": {
            "sourceType": "module"
        },
        "plugins": {
            // Config array element owns the loaded plugins.
            "@typescript-eslint": {
                "definition": { ... }, // the plugin implementation.
                "id": "@typescript-eslint",
                "filePath": "node_modules/@typescript-eslint/eslint-plugin/dist/index.js",
                "importerPath": "node_modules/@typescript-eslint/eslint-plugin/dist/index.js"
            }
        },
    }
]

If arbitrary errors happen while loading a plugin or a parser, the config array stores the error information rather than throws it. Because the plugin or the parser might not be used finally. When ConfigArray#extractConfig(filePath) method extracted configuration for a file, if the final configuration contains the error, it throws the error. Here, "extract" means merge the elements in the config array as filtering it by files property and excludedFiles property.

ℹ️ User-facing change:
Even if unused dependencies have some errors, ESLint doesn't throw it. (fixes eslint/eslint#11396)
💡 Example: 
[
    {
        "name": ".eslintrc.json » plugin:@typescript-eslint/recommended",
        "filePath": "node_modules/@typescript-eslint/eslint-plugin/dist/index.js",
        // Config array element owns the loaded plugins.
        "parser": {
            "error": Error, // an error object (maybe "Module Not Found").
            "id": "@typescript-eslint/parser",
            "importerPath": "node_modules/@typescript-eslint/eslint-plugin/dist/index.js"
        },
        "parserOptions": {
            "sourceType": "module"
        },
        "plugins": {
                // Config array element owns the loaded plugins.
            "@typescript-eslint": {
                "definition": { ... },
                "id": "@typescript-eslint",
                "filePath": "node_modules/@typescript-eslint/eslint-plugin/dist/index.js",
                "importerPath": "node_modules/@typescript-eslint/eslint-plugin/dist/index.js"
            }
        },
    }
]

If Linter#verify received a ConfigArray object, it requires options.filename as well. The Linter object calls ConfigArray#extractConfig(filePath) method and set needed parser, rules, and environments up. If the options.filename was /path/to/<INPUT>.js, it gives each rule only <INPUT> part.

📝 Note:

Now we can implement #3 in Linter#verify method because the ConfigArray object has the complete information to handle virtual files. So we get two pros.

  • We don't need to access to the file system for each virtual file.
  • We don't need to clone the logic of Linter#verifyAndFix method.

3. It changes the processing order to "config-then-files" from "files-then-config".

Currently, first it finds target files by globs, next it finds configs for each target file. Therefore, we could not change target files by configuration files because of this ordering.

This proposal changes that ordering.

  1. When the file enumerator entered into a directory,
    1. It finds .eslintrc.* file on the directory.
      • If found, it concatenates the found configuration to the parent configuration (just Array#concat).
    2. It enumerates files on the directory.
      • ✨ If a file is a regular file and matched to the current criteria, yields the pair of the file and the current configuration.
      • If a file is a directory, enters into the directory (go step 1).

lib/lookup/file-enumerator.js#L303-L360 in PoC.

📝 Note:
As a side effect, the file enumerator reuses configuration instances naturally without a special cache logic.

As the result, we can change target files by settings of configuration files.

4. It restructures the files about CLIEngine and lookup.

It's premature optimization if we moved a logic only one functionality is using to the shared utility directory. The shared utility directory is similar to global variables, so it makes hard to know who use the utilities.

This proposal moves some utility files to the directory of lookup functionality.

We should be able to understand the lookup logic only the files in lib/lookup.

Documentation

This core proposal needs migration guide because of a breaking change by a bug fix.

  • If people was affected by the overrides order change, they have to modify their config file.

Additional enhancements need documents as well:

Drawbacks

This is a large change. Currently we have multiple pull requests around configuration files, this will conflict with those pull requests severely. And this will be a large change for tests. Our tests really depend on internal structures seriously.

Backwards Compatibility Analysis

Most cases work fine as is.

✅ CLIEngine#addPlugin

It can work fine as is. CLIEngine has a Map<string, Plugin> for the added plugin, and it gives FileEnumerator (then ConfigArrayFactory.load* methods) the map, and the loading logic uses the map to load plugins.

✅ Linter

It can work fine as is.

Only if a given config has extractConfig method, Linter does the setup process for the config. Otherwise, it works the same as currently.

See this paragraph also.

✅ LintResultCache

It can work fine as is.

The ConfigArray has all information to distinguish if the config was changed since the last time.

⚠️ Fix a surprised behavior of overrides.

Currently, overrides is applied after all overrides properties are merged. This means that the overrides in a shareable config priors to the setting in your .eslintrc.

extends:
    - foo
rules:
    eqeqeq: error # silently ignored if the "foo" has the "eqeqeq" setting in "overrides".

After this proposal, the setting in your .eslintrc priors to the setting in shareable configs always. This is a breaking change, but I think this is a bug fix.

⚠️ About additional enhancements

An additional enhancement have a breaking change.

Alternatives

  • #9 is the alternative. But double duplicate features cause confusion for the ecosystem. For newcomers, a mix of articles about two config systems makes hard to understand ESLint. For non-English users, the official document is far.

Open Questions

Frequently Asked Questions

Related Discussions

Especially, this proposal is inspired by the discussion on #9 and #14.