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

no-unnecessary-else auto-fixer interacts badly with prettier #106

Open
Swatinem opened this issue Jan 24, 2019 · 2 comments
Open

no-unnecessary-else auto-fixer interacts badly with prettier #106

Swatinem opened this issue Jan 24, 2019 · 2 comments

Comments

@Swatinem
Copy link

We have a pre-commit hook that runs tslint --fix, but with a recent update of this plugin that does auto-fixing for no-unnecessary-else, it generates really strange code, most likely because it interacts in a bad way with prettier.

Some examples:

-    } else if (pageLoadingError) {
+    }  if (pageLoadingError) {

It just removes the else keyword, but it does not move the remaining if to a new line, instead it now has two spaces between } and if.

-    } else {
+    }  {

Here, it also just removes the else keyword, but leaves the whole block there, also with the mentioned whitespace issues like before.
This case is admittedly more complex, since you could have block-scoped let/const bindings that could lead to breakage when you just remove the whole block, so in this case developer intervention is still required.

@ajafff
Copy link
Owner

ajafff commented Jan 24, 2019

I agree that this is not the ideal behavior.
The rule currently tries to only remove the necessary tokens from your code. It doesn't handle formatting (or reformatting) at all.

This is done for multiple reasons:

  • projects have very different preferences regarding placement of statements, braces, keywords
  • it avoids logic to determine the correct indentation of every nested line
  • should avoid most cases of ASI errors when fixing semicolon-less code

The second case you mentioned is really because the block contains block-scoped declarations. The fixer could be smarter by analysing if the outer scope contains references to the shadowed binding (which would change by removing the block).

I'm accepting PRs to change the current behavior. Although I think this should really be done by a formatter.
Speaking of which: you mention Prettier in the issue title. What does it have to do with the fixer of this rule? Ideally it would simply reformat the fixed code according to your settings.

@Swatinem
Copy link
Author

Although I think this should really be done by a formatter.
Speaking of which: you mention Prettier in the issue title. What does it have to do with the fixer of this rule?

Well yes, we use both this rule and the tslint prettier plugin, which in this case does not fix the strange formatting left behind by this rule.
I think this is an ordering issue. And I’m not sure how I can reorder rules or control which one runs after the other.

This has led to quite some confusion because:

  1. tslint --fix runs in a pre-commit hook, where apparently no-unnecessary-else runs after prettier and leaves behind the strange formatting.
  2. tslint runs again and fails on CI because things are not formatted correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants