Pass options to broccoli-lint-eslint

[michalsnik]

Hi,

I encountered a problem with eslint today:

• I was using https://github.com/netguru/eslint-plugin-netguru-ember
• One of the rules needed improvement because eslint was yelling in one case when it shouldn't
• I fixed the it, released new version of plugin and updated package in my project
• Eslint from command line worked as expected - no problems
• Eslint called by ember was still showing errors, however when I changed anything in given "wrong" file - it automagically started passing

I found out that there is a setting called persist in broccoli-lint-eslint.
Funny thing, documentation says the default value is false, but the code clearly shows otherwise. But straight to the point..

In broccoli-lint-eslint there is:
https://github.com/ember-cli/broccoli-lint-eslint/blob/bf627753313b0df664c2ef69f71833925d0cfe62/lib/index.js#L102

if (typeof this.internalOptions.persist === 'undefined') {
    this.internalOptions.persist = true;
}

the this.internalOptions contains all the settings that are being passed through ember-cli-eslint in (https://github.com/ember-cli/ember-cli-eslint/blob/master/index.js#L32), unfortunately ember-cli-eslint doesn't passes settings from ember-cli-build.js.

This is really simple PR, that makes sure the settings are passed to broccoli-lint-eslint so users can leverage from using settings like persist and so on like this:

var app = new EmberApp(defaults, {
    eslint: {
      options: {
        persist: false,
      }
    },
  });

This PR also sort of a continuation of original Issue: #84

Looking forward to hearing from you :)

[Turbo87]

@michalsnik thanks for working on this. I do however believe that this might not be the best solution for the problem. The problem as you've found out is that broccoli-lint-eslint is caching the results of the checks, but apparently the cache invalidation logic does not seem to account for updating ESLint plugins. IMHO it would be best to fix the cache invalidation code instead of exposing settings here to hide the problem by disabling the cache and introducing performance issues instead.

[michalsnik]

Thanks for a reply @Turbo87.
I totally agree that the best solution would be to deal with the cache invalidation logic, and I'd also rather omit linting all files when it's not necessary.
I did however thought that it's probably not as quick task as it seems to be and it involves some research to be done so I came here with a quick solution that works, but as you said unfortunately doesn't resolve the cause of problems. I also though that it would be a good thing to allow users to take advanted of broccoli-lint-eslint settings, but it seems like you'd like to encapsulate everything in ember-cli-eslint so it just works out of the box and user shouldn't even care what's going on under the hood right?
I'll try to investigate this further and maybe I'll come back with more proper solution to this problem. (probably not in this repo)

Right now due to this issue my projects' build on CircleCI doesn't pass and I can't deploy in proper manner. I could slightly modify files that are not being checked right now so the cache invalidation works and the build finally will pass, but I'm thinking if there is a way to clean the cache and force it to check all of my files once again without the need to modify them?

[BrianSipple]

I do think there's a good point here about the ability to define ESLint-specific options.

broccoli-lint-eslint itself isn't doing anything with them other than passing them through to ESLint, and so I think it would make total sense for us to allow that much. Something like...

var app = new EmberApp(defaults, {
    eslint: {
      eslintOptions: {
        .....
      }
    },
  });

and then, the only change for this PR would be:

 options: this.options.eslintOptions

[xdumaine]

This may help with #150

[tamzinblake]

I totally need this for #150 - can I help with this PR?

[michalsnik]

Do you want me to update this PR regarding @BrianSipple suggestion @Turbo87?

[Turbo87]

I'm a little worried about exposing the API of a second-level dependency directly. For solving #150 it seems that a rulesDir option in ember-cli-eslint be sufficient and we could even specify a default for that if we wanted to.

[tamzinblake]

How about #197 then?

[ctcpip]

It would be really helpful to have this - either passing through the options, or just implementing them as options explicitly like in #197. In the meantime, is there any way to do this, hacky as it may be? With jsHint, I was able to do this:

ember-cli-build.js

const jsHint = require('broccoli-jshint');
jsHint.prototype.someOption = true;

[Turbo87]

closing in favor of #197

[ctcpip]

@Turbo87 please reopen this issue - #197 does not satisfy the general requirement for passing through options

[Turbo87]

@ctcpip what option do you want to pass through?

[ctcpip]

@Turbo87 me specifically, throwOnError and throwOnWarn. But I think the best solution is to just pass through the entire options object. Just like broccoli-lint-eslint does to pass through options to ESLint CLIEngine. As an aside, it's a pretty common paradigm to do it that way for wrappers in general.

[Turbo87]

but throwOnError doesn't actually make sense in the context of ember-cli-eslint. do you want Ember CLI to crash during ember serve each time an ESLint error was discovered?

[ctcpip]

that's exactly what I want