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

Disable arrow-body-style rule #71

Closed
sharmilajesupaul opened this issue Jan 14, 2019 · 14 comments
Closed

Disable arrow-body-style rule #71

sharmilajesupaul opened this issue Jan 14, 2019 · 14 comments

Comments

@sharmilajesupaul
Copy link

When using the arrow-body-style rule along with the Prettier, the output can result in invalid code.
Here's a thread with a working example: airbnb/javascript#1987

Since prettier handles arrow style well, can we switch this rule off in this repo?

arrow-body-style config that results in invalid code:

['error', 'as-needed', { requireReturnForObjectLiteral: false, }],

code

const foo = (paramA, paramB, paramC) => {
  return Object.keys(paramB.prop.nestedProp[0])
    .filter(attribute => paramC.includes(attribute))
    .map(attribute => {
      return Object.keys(paramB.prop.nestedProp[0][attribute]).map(
        attributeKey => {
          return {
            key: `${paramA.name} ${paramB.type} ${attribute} ${attributeKey}`,
            value: paramB.prop.nestedProp[0][attribute][attributeKey],
          }
        }
      )
    })
}

eslint config:

{
  "parser": "babel-eslint",
  "plugins": ["prettier", "react", "jest"],
  "env": {
    "browser": true,
    "node": true,
    "jasmine": true,
    "jest/globals": true
  },
  "parserOptions": {
    "ecmaVersion": 2017,
    "sourceType": "module",
    "ecmaFeatures": {
      "jsx": true
    }
  },
  "settings": {
    "import/resolver": {
      "webpack": {
        "config": "webpack.common.js"
      }
    }
  },
  "rules": {},
  "extends": ["airbnb", "plugin:prettier/recommended"]
}
@brodybits
Copy link

I suspect they wouldn't mind a quick PR:)

@lydell
Copy link
Member

lydell commented Jan 14, 2019

I think this is another manifestation of prettier/eslint-plugin-prettier#65

@lydell
Copy link
Member

lydell commented Jan 14, 2019

In other words, I expect this issue to play out like this one: #31

@lydell
Copy link
Member

lydell commented Jan 14, 2019

You should be able to manually add the missing paren and then it should work fine.

@ljharb
Copy link

ljharb commented Jan 14, 2019

I don't see how it's acceptable to suggest that a valid workaround is to fix the invalid JS that prettier outputs :-/ it's reasonable to ask users to apply the prettier config (disabling conflicting rules) - why not include this one in it?

@lydell
Copy link
Member

lydell commented Jan 14, 2019

the invalid JS that prettier outputs

As far as I understand, Prettier does not output invalid JS. (Try it with Prettier only!) It’s ESLint’s --fix functionality that has a bug, where different fixers from different rules can sometimes mess up the output. This issue can only happen if you use eslint-plugin-prettier. Run the tools separately and it won’t happen (unless I’m mistaken).

So the way I see it, arrow-body-style is compatible with Prettier. A fix should go to ESlint (and/or eslint-plugin-prettier, possibly).

Does that make sense?

@ljharb
Copy link

ljharb commented Jan 14, 2019

Sure, it's clearly a bug with the combination of eslint and prettier - but that's precisely what this config is meant to address, disabling the rules that conflict.

I don't have an opinion on where the fix should go, but it's very urgent imo to stop eslint-plugin-prettier from outputting incorrect code, so if adding a line to this config (that disables a rule that prettier subsumes anyways) is faster than getting a proper fix into the plugin (like prettier/eslint-plugin-prettier#65 (comment), eg), then that seems like the most prudent course of action.

@lydell
Copy link
Member

lydell commented Jan 14, 2019

The thing is, this could happen to any rule with autofix. So far we’ve run into 2 rules that trigger this issue. Hopefully that’s the only two, but who knows.

Maybe we should put them into the “Special rules” category so that the CLI helper tool warns about them.

@ljharb
Copy link

ljharb commented Jan 14, 2019

Sure :-) I'm not suggesting a general solution, just something that fixes the issue with these two known rules.

@sharmilajesupaul
Copy link
Author

sharmilajesupaul commented Jan 14, 2019

I like the idea of logging a warning in the CLI tool for rules that cause this problem because it would at least surface the issue to developers who can then choose to either turn the rule off or manually fix.
There are times, like when mass refactoring using prettier+eslint, when just adding the paren and moving forward may not be so simple.

@lydell
Copy link
Member

lydell commented Jan 14, 2019

There are times, like when mass refactoring using prettier+eslint, when just adding the paren and moving forward may not be so simple.

(Actually, it is. Run the tools separately. You don’t have to add the paren manually.)

@ljharb
Copy link

ljharb commented Jan 14, 2019

Telling devs to run the tools separately isn't tenable for us, either.

@lydell
Copy link
Member

lydell commented Jan 26, 2019

arrow-body-style (and prefer-arrow-callback) are now disabled by default in v4.0.0. See the docs for more information.

@lydell lydell closed this as completed Jan 26, 2019
@polomoshnov
Copy link

I opened a similar issue in React Boilerplate before I learned from this repo's readme that it's a known bug.

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

No branches or pull requests

5 participants