-
-
Notifications
You must be signed in to change notification settings - Fork 179
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
prefer-arrow-callback fix conflicting with prettier fix #65
Comments
It could also be how ESLint applies fixes. |
Related: eslint/eslint#7928 tl;dr: Autofixes from rules can, on rare occasions, conflict with each other. Rules can work around the issue by coarsening their fixes to ensure that the "fix range" contains the entire range of text that caused the particular problem to be reported, rather than just the actual text replacement. For convenience, could you paste the output of running ESLint without |
Here is the output of running
With |
Thanks for clarifying. This is actually a bug in It would probably be pretty difficult to determine which fixes need to be applied together as part of |
Are there any plans to fix this? |
I believe the only way to fix this - changing the plugin so every change is batched into a single "fix" - results in a much worse user experience than the current behaviour of multiple smaller fixes in the common case of where you have multiple small and independent fixes. I don't think the tradeoff is worth it so I think this will end up remaining unfixable. |
Not necessarily - it could just be sure to specifically replace entire arrow functions at once, no? |
I think there’s one more way to fix this – taking a really deep dive into the ESLint autofix code and trying to fix the problem at its core. Sounds harder though. |
What's the differences in user experience and why do you consider them to be worse? |
Thanks for calling me out on that, I knew I was being lazy with that handwaving :) Currently this plugin does the following:
This means that if you have a file like: const a = 0;
const b=0;
const c = 0;
const d=0;
const e = 0; it will report changes (adding the spacing around the equals) on lines 2 and 4. const a = 0;
-const b=0;
+const b = 0;
const c = 0;
-const d=0;
+const d = 0;
const e = 0; This "find the smallest block of changes" logic gives a nice DX experience as it means the smallest change a user needs to make is shown. The naive shotgun approach that would solve the problem, but give a crappy DX is to not try and split out changes into atomic parts, and instead batch all the changes into one big one. In this case the suggested fix for the above code would be: const a = 0;
-const b=0;
-const c = 0;
-const d=0;
+const b = 0;
+const c = 0;
+const d = 0;
const e = 0; Note that the While this approach is simple to implement the potential for making hard to understand suggestions that make it untenable IMO. The 3rd approach is @ljharb's - a diff mechanism that understands JS and can block changes within a function into a single change. This would solve the problem but a very quick search came up with no drop-in solution available at the moment, and my gut thinks implementing a diffing algorithm that understands JS logic would be complex and much slower than the current algorithm. If there's a way that this can happen without adding too much complexity to our codebase and doesn't have a major performance impact then I'd be interested in reviewing a PR but I don't have much interest in attempting to solve this personally as my gut thinks it's going to be a lot of effort and a lot of complexity for worse overall performance for a problem that only crops up occasionally. Happy to be proved wrong though :) |
I opened a similar issue in React Boilerplate. |
Is it possible to know when we are running in auto fix mode and decide to create a single fix instead of individual fixes? This way the DX of showing the smallest diffs will still exist when running without --fix for example with the eslint vscode plugin, but when it comes to auto fixing all rules there will only be one fix to apply so it can't conflict with other fixes. |
I wonder if |
Or maybe we can introduce a |
That's what I was thinking, but I don't know if it works. |
I think I found another rule that can cause problems:
export default class Root extends Component {
getInitialState = () => ({
errorImporting: null,
errorParsing: null,
errorUploading: null,
file: null,
fromExtension: false,
importSuccess: false,
isImporting: false,
isParsing: false,
isUploading: false,
parsedResults: null,
showLongRunningMessage: false,
});
} Here's eslintrc and package.json: |
Why would that rule cause problems? Prettier is about formatting; that rule is about actual runtime semantics. |
Produces this code: export default class Root extends Component {
getInitialState() { return {
errorImporting: null,
errorParsing: null,
errorUploading: null,
file: null,
fromExtension: false,
importSuccess: false,
isImporting: false,
isParsing: false,
isUploading: false,
parsedResults: null,
showLongRunningMessage: false,
}; }); // Extra trailing paren
} |
Probably this is what it should produce: export default class Root extends Component {
getInitialState() {
return {
errorImporting: null,
errorParsing: null,
errorUploading: null,
file: null,
fromExtension: false,
importSuccess: false,
isImporting: false,
isParsing: false,
isUploading: false,
parsedResults: null,
showLongRunningMessage: false,
};
}
} I had to manually delete trailing paren to get it to format correctly |
@devinrhode2 that sounds like a bug in the rule's autofix; i'd appreciate you filing an issue on eslint-plugin-react :-) |
yeah it actually seems totally unrelated to prettier. Tried disabling all the prettier stuff, run just eslint autofix, and still happening. |
any news7 |
Edit by @lydell: TL;DR We recommend turning off these rules for the time being:
What version of
eslint
are you using?v4.9.0
What version of
prettier
are you using?v1.7.4
What version of
eslint-plugin-prettier
are you using?v2.3.1
Please paste any applicable config files that you're using (e.g.
.prettierrc
or.eslintrc
files)https://github.com/ismail-syed/prettier-eslint-config-invalid-code
What source code are you linting?
What did you expect to happen?
The code above should be formatted as per prettiers config and also should adhere to that
prefer-arrow-callback
fixWhat actually happened?
Invalid code was generated, closing parenthesis is missing on the return statement.
Is the underlying issue from the
prefer-arrow-callback
fixer or the prettier plugin fixer?The text was updated successfully, but these errors were encountered: