Prettier-eslint: Possible Regression or Major Change?

[robwise]

Hey there, I'm getting a bunch of conflicting reports on the prettier-atom repo about people's prettier-eslint all the sudden not respecting their config: https://github.com/prettier/prettier-atom/issues/334#issuecomment-354759861

Was there a breaking change or possibly a regression somewhere in the 8.5.0 release? These reports started coming in when I bumped the bundled version that comes with prettier-atom.

[azz]

Just a guess: https://github.com/prettier/prettier-eslint/commit/5bd3684

[zimme]

I'll look into it today.

[zimme]

Closing with the following message.
https://github.com/prettier/prettier-atom/issues/334#issuecomment-355864770

Ping me if this doesn't resolve the issue.

[zimme]

It seems 8.7.0 wasn't the problem. However there was a problem there.

[ncknuna]

@zimme I just ran into this and debugged a bit. I think it's actually https://github.com/prettier/prettier-eslint/commit/bed4374ef1f86275cbb6210a49013bf73ff126f7#diff-2b4ca49d4bb0a774c4d4c1672d7aa781, which adds the concept of dynamically determining unfixable rules. While good in concept, this line presents some problems:

const linter = new Linter(eslintConfig);

because, AFAICT, Linter's constructor doesn't take any arguments (https://github.com/eslint/eslint/blob/88d5d4dbd09dd7f23e9fff79087bc13adaac051b/lib/linter.js#L710), which means the existing config gets lost, and only the default rules get loaded (https://github.com/eslint/eslint/blob/88d5d4dbd09dd7f23e9fff79087bc13adaac051b/lib/rules.js#L63), thereby dropping all rules from plugins being considered as "fixable" rules. Since they aren't considered fixable, they get omitted here:

https://github.com/prettier/prettier-eslint/blob/master/src/utils.js#L91

[zimme]

@ncknuna, Thank you for looking into this.

It seems that you are correct in the fact that Linter does not accept an argument. Which I will remove as it's not needed.

However, even if I do remove the parameter to Linter the code will still work the same way as new Linter().getRules() will give me the same set of rules, i.e. all rules eslint can find from its own set of default rules and all the rules any plugins provides.

I use this map of rules to find out if the rules I iterate over here is fixable.

[zimme]

Is there a chance that there are fixable rules that don't have meta.fixable property set?

[ncknuna]

@zimme, you're right that if you remove that parameter to Linter that the code will work the same, but at least from my inspection of the logic behind the Linter constructor and the getRules call (and from sticking debugger statements into the code and stepping through it), doing new Linter().getRules() won't load any plugins, which is the real problem. In other words, I don't think the second part of your statement i.e. all rules eslint can find from its own set of default rules and all the rules any plugins provides is true.

The easiest way I can thinking of getting all of the rules that are provided by all plugins is to update index/getESLintConfig to return both the config and the cliEngine, then pass the cliEngine into utils.getOptionsForFormatting -> getRelevantEslintConfig -> getFixableRules and then instead of using Linter().getRules(), use cliEngine.getRules() or cliEngine.linter.getRules()

[noisysocks]

I'm encountering this issue as well.

In my debugging, I noticed that one of my rules that's defined by a plugin does not appear in the array produced by getFixableRules(). This causes prettier-eslint to omit it from the relevant rules and not apply the fix. You can see here that the rule definitely has meta.fixable set.

@ncknuna's explanation on why this happens seems correct to me.

[zimme]

Nice catch @ncknuna and @noisysocks, I hope to have a fix out today. eslint 4.15.0 was released just a few days ago and that version should have getRules on CLIEngine which we could use to fix this.

[ncknuna]

@zimme I think the options that the Linter expect are different than the options that the CLIEngine expect, so one isn't just a drop-in replacement for the other. After updating to the newest version and running with trace-level logging, I got the following error:

prettier-eslint [ERROR]: There was trouble creating the ESLint CLIEngine.
prettier-eslint-cli [ERROR]: There was an error formatting "<my file>": 
    TypeError: (options.globals || []).reduce is not a function
        at new Config (.../node_modules/eslint/lib/config.js:93:48)
        at new CLIEngine (.../node_modules/eslint/lib/cli-engine.js:420:23)
        at getESLintCLIEngine (.../node_modules/prettier-eslint/dist/utils.js:478:12)
        at getFixableRules (.../node_modules/prettier-eslint/dist/utils.js:159:13)
        at getRelevantESLintConfig (.../node_modules/prettier-eslint/dist/utils.js:130:22)
        at getOptionsForFormatting (.../node_modules/prettier-eslint/dist/utils.js:122:16)
        at format (.../node_modules/prettier-eslint/dist/index.js:94:62)
        at MapSubscriber.project (.../node_modules/prettier-eslint-cli/dist/format-files.js:305:55)
        at MapSubscriber._next (.../node_modules/rxjs/operators/map.js:79:35)
        at MapSubscriber.Subscriber.next (.../node_modules/rxjs/Subscriber.js:90:18)
failure formatting 1 file with prettier-eslint

[zimme]

Aah yeah, it's that problem again. The globals option needs to be an array as that's what's expected in CLIEngine and there's been a comment about this. I do believe this needs to be fixed in prettier-eslint-cli.

[zimme]

@ncknuna, check if the latest version fixes your issue.

[ncknuna]

@zimme Getting there! That latest version does fix my issue, but still doesn't include the rules from plugins. It seems that this getConfigForFile call is what actually loads the plugins in the other place that CLIEngine is used (https://github.com/prettier/prettier-eslint/blob/master/src/index.js#L230), which I determined by sticking a debugger statement before that line and checking cliEngine.getRules() before and after that line. Seems like the most obvious solution would be to pass the file name through those layers of calls, or the other CLIEngine instance like I mentioned above.

[zimme]

I think I'll just memoize getESLintCLIEngine based on both parameters.

[ncknuna]

Go for it; I don't have a strong preference on implementation details, I just want my plugin rules! :)

[ncknuna]

@zimme Any updates? Hoping to get this all sorted out by end of week :)... If it's going to be a little while before it can get fixed, we should probably re-open the issue so other people can find it

[zimme]

I'll look at this later tonight, still at work. If I can't find a good way
to do this tonight I'll just remove the fixable rules filtering until I
can find a proper solution.

[ncknuna]

Awesome, thanks!

[zimme]

I'll be removing the filter fixable rules logic until I can do some refactoring.

[zimme]

@ncknuna Test the latest version.

I'm getting some warnings now, but that seems to be related to prettier 1.10+.

I'll open an issue over there.

[ncknuna]

Yay, fixed for me! Thanks so much! :D

[zimme]

I'm thinking that some of the reported issues here are because of some error in the following logic. https://github.com/prettier/prettier-eslint/blob/master/src/index.js#L99-L117.

[zimme]

Version 8.7.6 is out and should, hopefully, finally, fix this properly.

[LuisValgoi]

@zimme I'm facing the same issue on "prettier-eslint": "^9.0.1" and "prettier-eslint-cli": "^5.0.0".

package.json

{
  "name": "..",
  "version": "0.1.0",
  "private": true,
  "dependencies": {
    ...
  },
  "scripts": {
    ..
  },
  "devDependencies": {
    "eslint": "^6.8.0",
    "prettier-eslint": "^9.0.1",
    "prettier-eslint-cli": "^5.0.0"
  }
}

command used

npx prettier-eslint src/*.js

error

[chenxiaoyao6228]

@zimme I'm facing the same issue on "prettier-eslint": "^9.0.1" and "prettier-eslint-cli": "^5.0.0".

It seems this issue is not fixed.