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

Support Lazy Loading Rules from 3rd Party Plugins #14862

Closed
bradzacher opened this issue Jul 31, 2021 · 6 comments
Closed

Support Lazy Loading Rules from 3rd Party Plugins #14862

bradzacher opened this issue Jul 31, 2021 · 6 comments
Assignees
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 enhancement This change enhances an existing feature of ESLint Stale triage An ESLint team member will look at this issue soon
Projects

Comments

@bradzacher
Copy link
Contributor

The version of ESLint you are using.
7.27.0

The problem you want to solve.

Large 3rd-party ESLint plugins can often define hundreds of rules.

Loading all of these rules if the user only configures one or two can be wasteful, especially if the rule sets up some global state as part of being loaded.

As an example - at Facebook our internal ESLint plugin defines ~570 rules.
Some of these rules are only enabled on parts of the codeabse and read configuration files from the repo that in some cases can be hundreds of KB (or even a few MB in some cases).
Or some rules read in GraphQL schema information (which at FB is sized in the gigabytes and takes tens of seconds to produce).

For 3rd-party plugins ESLint will explicitly and forcefully iterate and resolve every single one of a plugin's rules, regardless of if they are actually used:
https://github.com/eslint/eslintrc/blob/9bf2ba12e6c0bfe7296774531fd79dea23d4c805/lib/config-array/config-array.js#L326-L337

OTOH, ESLint wraps its core rules object in a LazyLoadingRuleMap which allows the engine to load in lint rules if and only if they are explicitly used by the user.
ESLint's engine is hard-coded to respects this lazy-loading style only for the core rules.

I've tried playing around with a few solutions in user-land, like using a Proxy:

const rules = {
  'rule': () => require('./rule');
};

module.exports = new Proxy(rules, {
  get(target, key) {
    if (key in target) {
      return target[key]();
    }
    return undefined;
  },
});

But because of ESLint's usage of Object.entries to collect and normalise all of a plugin's rules - it is no different to just defining an object.

Your take on the correct solution to problem.
ESLint should treat 3rd-party plugins the same as its core rules - it should not iterate the set of plugin rules automatically and should instead only access the rules that the configuration explicitly enables.

Alternately ESLint could expose its LazyLoadingRuleMap utility publicly and then do feature detection like plugin.rules instanceof LazyLoadingRuleMap to conditionally enable the lazy-loading feature, in case you are concerned about an unintentional breaking change.

Are you willing to submit a pull request to implement this change?
I'm short on OSS time, but I could look into it at some point.

@bradzacher bradzacher added core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint triage An ESLint team member will look at this issue soon labels Jul 31, 2021
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Jul 31, 2021
@nzakas nzakas moved this from Needs Triage to Feedback Needed in Triage Aug 2, 2021
@nzakas
Copy link
Member

nzakas commented Aug 2, 2021

This is an interest idea, though I think you can probably accomplish this right now by defining getters instead of properties. For example:

module.exports = {
    rules: {
        get "my-rule"() {
            return require("./lib/rules/my-rule");
        }
}

Because require() calls are cached automatically, you wouldn't even need to check if the rule had already been loaded. I think this would solve your problem and should work seamlessly in ESLint because ESLint only ever reads these keys and never writes to them.

@bradzacher
Copy link
Contributor Author

though I think you can probably accomplish this right now by defining getters instead of properties

You cannot - as mentioned in OP:

For 3rd-party plugins ESLint will explicitly and forcefully iterate and resolve every single one of a plugin's rules, regardless of if they are actually used:
https://github.com/eslint/eslintrc/blob/9bf2ba12e6c0bfe7296774531fd79dea23d4c805/lib/config-array/config-array.js#L326-L337

This means that attempting to do this in user-land does not work because ESLint gets all rules defined by the plugin when it is loaded so it can normalize the rule format.

@nzakas
Copy link
Member

nzakas commented Aug 3, 2021

Ah sorry, was reading on my phone and missed that.

Good news/bad news: Bad news, we aren’t changing eslintrc, so there’s nothing we can do in the near term to address this. Good news, the new config format will allow what I described in my comment. So we will get there eventually, just not immediately.

@aladdin-add
Copy link
Member

aladdin-add commented Aug 11, 2021

or allowing the rule to be a path:

// plugin
module.exports = {
  rules: {
    'some-rule': require.resolve('./some-rule.js'),
  }
}

when loading the plugin in eslint:

const rule = typeof rules[ruleId] === 'string' 
  ? require(rules[ruleId]) // load the rule
  : rules[ruleId]; // as-is

@nzakas nzakas moved this from Feedback Needed to Blocked in Triage Sep 29, 2021
@github-actions
Copy link

Oops! It looks like we lost track of this issue. @eslint/eslint-tsc what do we want to do here? This issue will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Oct 15, 2021
@nzakas nzakas self-assigned this Oct 16, 2021
@nzakas
Copy link
Member

nzakas commented Oct 16, 2021

I’m still planning on addressing this in the new config system. I think it’s actually already working in my prototype but haven’t validated it yet.

Triage automation moved this from Blocked to Complete Dec 4, 2021
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jun 3, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jun 3, 2022
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 enhancement This change enhances an existing feature of ESLint Stale triage An ESLint team member will look at this issue soon
Projects
Archived in project
Triage
Complete
Development

No branches or pull requests

3 participants