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

Could shareable configs and plugin configs be executed more than once? #8943

Closed
platinumazure opened this issue Jul 13, 2017 · 7 comments
Closed
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion feature This change adds a new feature to ESLint question This issue asks a question about ESLint

Comments

@platinumazure
Copy link
Member

My use case is a little odd, so I'll start with the background.

I'm working on a plugin, eslint-plugin-editorconfig. The goal of the plugin is to export a configuration which will basically hunt down .editorconfig files, read them, and convert the editorconfig rules into ESLint rule configurations. It seems to work pretty well for simple projects that have one .editorconfig file in the project root directory.

The problem I'm dealing with is, I don't know how to support editorconfig's cascading nature. Like ESLint, .editorconfig files can be added to subdirectories, and implementations can then read all ancestor .editorconfig files and apply the combined settings. However, ESLint (or rather, Node require()) reads plugins and shareable configs once. So it doesn't seem like there is a way for ESLint to "reread" the plugin-exported config and look for a possibly different set of .editorconfig files, such that the ESLint rules exported in that configuration would be appropriate to the subdirectory with the overriding .editorconfig file.

Am I missing something? Is there a way to make this work with existing ESLint capabilities?

If there isn't a way to make it work, I did have a pretty simple idea in mind. Basically, configurations (whether exported by plugins or published as shareable configs) could optionally export a function instead of an object, and if they export a function, then the function would be run for every linted file (presumably given an options object with things like the linted file's path). In that case, I could use that path instead of process.cwd() when asking the editorconfig implementation to look for .editorconfig files, and it would simply recompute the configuration on every file. (Of course, performance would probably tank unless I implemented a caching layer, but it would at least support my use case.) But I'm open to better ideas as well.

@platinumazure platinumazure added core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion feature This change adds a new feature to ESLint question This issue asks a question about ESLint labels Jul 13, 2017
@not-an-aardvark
Copy link
Member

not-an-aardvark commented Jul 13, 2017

Would #8813 solve this issue? That would allow the config to specify overrides for particular files depending on the .editorconfig, although it would still have to be exported statically.


Another interesting way to do this would be to have a single rule for your plugin, and use composition:

"use strict";

const eslint = require("eslint");

module.exports = {
    create(context) {
        const filename = context.getFilename();

        // parse and convert the .editorconfig files as necessary, etc.
        const eslintConfig = getConfigFor(filename);

        return {
            Program() {
                const linter = new eslint.Linter();

                // Run eslint on the file with the given config, and report the problems
                linter.verify(context.getSourceCode(), eslintConfig).forEach(problem => {
                    context.report({
                        loc: { line: problem.line, column: problem.column },
                        message: "{{ruleId}}: {{message}}",
                        data: problem,
                        fix: problem.fix
                    });
                });
            }
        }
    }
};

I like this strategy because the idea of composing rules is intriguing, and now I'm wondering what other problems could be solved using rule composition. The downside is that all problems will get reported from the same rule (and inserting the actual rule ID into the message doesn't seem like a great solution). Also, this would require another AST traversal, but maybe it would be possible to avoid that by attaching the rule listeners directly rather than just using one Program listener.

@platinumazure
Copy link
Member Author

platinumazure commented Jul 13, 2017

@not-an-aardvark That's an intriguing option, but it honestly feels like a mixing of responsibilities there.

Also, and this is probably a dealbreaker, if a user explicitly specifies overrides for the ESLint rules in question, then both versions of the rule would run:

{
    "plugins": ["editorconfig"],
    "rules": {
        "editorconfig/lint-all-the-things": "error",
        "indent": ["error", 4]
    }
}

If the .editorconfig file has indent set to tabs, so that editorconfig/lint-all-the-things effectively runs "indent": ["error", "tab"], then the configured indent rule also runs with "indent": ["error", 4]. In that case, the user would see lint errors no matter which way they indented.

Better (IMO) to handle this in config and then let ESLint's config cascading/hierarchy handle the nested configurations, resulting in one configuration for indent. (Analogous arguments can be made for the other rules supported by the eslint-plugin-editorconfig plugin-exported configuration.)

@platinumazure
Copy link
Member Author

Oh, and re: #8813, two problems with this: One, I would have to add glob entries for every subdirectory that has an .editorconfig file; two, I would still need to know what directory ESLint is "currently looking at" when generating a configuration in the plugin. (I certainly want to remove the hardcoded process.cwd(), but I wouldn't know what to replace it with and I also don't see how it would be able to change as ESLint goes into subdirectories without ESLint explicitly calling the plugin/config code somehow. I could be wrong, though.)

@platinumazure
Copy link
Member Author

Curious if anyone else on @eslint/eslint-team has any thoughts on this... Thanks!

@ilyavolodin
Copy link
Member

I don't think this can be done with existing ESLint code. I was thinking that you might be able to do it with a rule, but we freeze config object, once it's created, so you will not be able to modify it from the rule. Configs and plugins are executed only once, because they will gather huge performance overhead on a large codebase.

@platinumazure
Copy link
Member Author

platinumazure commented Jul 19, 2017

@ilyavolodin Could we enhance core to support a config exporting a function? It would be backward compatible and would not have performance impact (barring a typeof check) on existing configurations. And yes, performance would not be great for configs that do this, but it would be an opt-in/"at your own risk" sort of thing, and config developers who choose to do this could implement a caching strategy.

Or, if there's an enhancement we could make that is less likely to endanger performance, I'm open to that too. I just haven't been able to think of anything else yet.

EDIT: Actually, using glob patterns (assuming they work correctly in exported configs), I could probably crawl the directories in advance and then generate a glob pattern config as needed. Maybe I don't need anything here after all... I'll go try that and close this if I can prove it out.

@platinumazure
Copy link
Member Author

I'm going to close this for now. I misunderstood something about the .editorconfig spec and basically there's now no good way to do this except to combine configs from the top down anyway, so it should be okay for my configuration to be loaded and executed once.

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion feature This change adds a new feature to ESLint question This issue asks a question about ESLint
Projects
None yet
Development

No branches or pull requests

3 participants