Fix: avoid loading core rules dynamically from FS in Linter

[petermetz]

This is a backward compatible change of the public API.
By default the Linter class' constructor instantiates a Rules instance
which (in its own constructor) has the side-effect of reading from the
file-system to obtain rule files that may have been put there by the
user. The issue with this is side-effect is that it breaks WebPack
bundles (at runtime they crash when the Linter gets instantiated). The
particular use-case that spawned this PR is running ESLint inside a
WebPack bundle deployed onto an AWS Lambda function. The Linter is
configured inline so there is no need to load any rules from the
file-system.

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[x] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)
The Linter and Rules classes constructors have a new flag. They have backward compatible default values.

Is there anything you'd like reviewers to focus on?
I know there is a Browserify build available already, but that's for the web platform. My use-case is about running ESLint in NodeJS, but doing so from code that was bundled (along with it's dependencies) by a bundler, WebPack in my case.

[not-an-aardvark]

Hi, thanks for the PR.

This does seem like an issue; Linter isn't supposed to touch the filesystem for the most part. However, I don't think adding an option is the best way to fix it -- instead we should make it so that Linter doesn't load things from the filesystem at all.

It seems like the filesystem access that happens in the Linter constructor occurs only for the purpose of loading ESLint's core rules, which are all in the lib/rules directory. So we could probably address the issue by having an index file with a hardcoded require call for each rule, and removing the load() call entirely. Incidentally, it seems like we do a similar thing automatically here as part of generating the browserify build.

(I vaguely recall a discussion from a long time ago about how loading core rules from the filesystem lazily might improve performance because we would avoid loading core rules that don't get used. However, it doesn't seems like we're loading core rules lazily right now anyway, so I don't think that should be a concern.)

[petermetz]

@not-an-aardvark
Thanks for the review. Updated the PR as per your suggestion. I hope this works.

Future idea to improve performance with lazy loading: If all the individual rules were exported as part of the public API then (at least in my use-case) it would be possible for the consuming code to cherry pick the rules they would like to import and consequently include in their final bundles. Right now if you include all built-in rules it's a 2 MB hit (size of the lib/rules directory)

[not-an-aardvark]

Thanks, this looks good. I just have a few small suggestions.

[gyandeeps]

However, it doesn't seems like we're loading core rules lazily right now anyway, so I don't think that should be a concern.

I thought we collected the paths to all the core rules and loaded them(require) only the one which gets used.

Its been a while, so things might have changed.

[not-an-aardvark]

@gyandeeps The rules are all loaded anyway as a result of this Linter#getRules call in the CLIEngine constructor.

[not-an-aardvark]

LGTM, thanks! Waiting a bit before merging in case anyone else wants to review.

[platinumazure]

LGTM in general, but left one small wording suggestion.

[platinumazure]

LGTM, thanks!

[ilyavolodin]

As far as I remember, we used to have code like that to load rules at some point in the past. It was removed, because it's an additional step that's required any time you are adding/removing a new rule. I'm slightly confused by the description of the problem, as you trying to load ESLint in WebPack as a module, or you are trying to compile ESLint using WebPack? If the latter - we do not officially support ESLint for any platform other then Node. We did say that we would make it easier to use WebPack to compile ESLint, but without creating any negative side-effects. Having yet another place where we keep repository of all rules seems like a negative side-effect to me.
(Also, since this is doing more then just fixing a bug, by introducing an extra step for creating rules, this should probably go through TSC approval).

[petermetz]

As far as I remember, we used to have code like that to load rules at some point in the past. It was removed, because it's an additional step that's required any time you are adding/removing a new rule. I'm slightly confused by the description of the problem, as you trying to load ESLint in WebPack as a module, or you are trying to compile ESLint using WebPack? If the latter - we do not officially support ESLint for any platform other then Node. We did say that we would make it easier to use WebPack to compile ESLint, but without creating any negative side-effects. Having yet another place where we keep repository of all rules seems like a negative side-effect to me.
(Also, since this is doing more then just fixing a bug, by introducing an extra step for creating rules, this should probably go through TSC approval).

I'm bundling a NodeJS 8.x application with WebPack. The application uses ESLint as a dependency and therefore ends up in the bundle itself as well. There is no compilation step, it's all ES6.

Normally I would agree with the idea that there should be a single source of truth for the rule definitions, but am okay with the index file because there's a build step checking it, failing the build if an inconsistency is detected. If there was a potential that this will end up creating buggy builds I would also oppose it, but that possibility is eliminated by the build-time check.

[not-an-aardvark]

We did say that we would make it easier to use WebPack to compile ESLint, but without creating any negative side-effects. Having yet another place where we keep repository of all rules seems like a negative side-effect to me.

For reference, the TSC resolution is here.

It's a matter of opinion, but I don't think having to list the rules is a big problem, provided that we have tests to ensure that the list is up-to-date. Core rules get added about once every few months nowadays -- it doesn't seem like a big deal to spend a minute adding a new rule to an index if the index serves a legitimate purpose.

(Also, since this is doing more then just fixing a bug, by introducing an extra step for creating rules, this should probably go through TSC approval.)

Our governance documentation says:

Changes to the core (including CLI changes) that would result in a minor or major version release must be approved by the TSC by standard TSC motion.

Requiring core rules to be added to a file in the ESLint repository is strictly a refactoring change, and it wouldn't result in a minor or major release. So it doesn't need TSC approval provided that any objections can be resolved on the issue.

[platinumazure]

@ilyavolodin FWIW, I'm hoping to come back here when this is merged and make it so we don't actually have to maintain the rule list in this new file.

[ilyavolodin]

@not-an-aardvark Agree on the TSC requirement.

Core rules get added about once every few months nowadays -- it doesn't seem like a big deal to spend a minute adding a new rule to an index if the index serves a legitimate purpose.

It is legitimate purpose, but not officially supported one, that's what make me pause.

@ilyavolodin FWIW, I'm hoping to come back here when this is merged and make it so we don't actually have to maintain the rule list in this new file.

In that case, I don't have objections.

[not-an-aardvark]

Ok, merging this PR since it's a bugfix and the objections have been addressed.

[not-an-aardvark]

Thanks for contributing!