Skip to content

fix(eslint-config): Clashes with prettier#26847

Closed
iRoachie wants to merge 1 commit into
facebook:masterfrom
iRoachie:master
Closed

fix(eslint-config): Clashes with prettier#26847
iRoachie wants to merge 1 commit into
facebook:masterfrom
iRoachie:master

Conversation

@iRoachie
Copy link
Copy Markdown
Contributor

Summary

Currently, @react-native-community config package extends from prettier/recommended which comes with default settings from prettier. However there are still some eslint rules in the config that either clash or duplicate the settings from prettier.

This results in eslint fixing the formatting and then prettier undoing it. This PR removes the style specific rules from eslint and place them in the prettier section.

Changelog

[General] [Fixed] - Remove style rules from eslint config for prettier options

Test Plan

I created a repo for you to test with https://github.com/iRoachie/eslint-bug-replicate. You can see that running yarn lint --fix will never fix the issue. Eslint will complain about double quotes and subsequently after fixing it will complain about single quotes.

Here's a gif of the behaviour (vscode eslint plugin "eslint.autoFixOnSave": true):

Kapture 2019-10-13 at 23 34 15

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 14, 2019
Copy link
Copy Markdown

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code analysis results:

  • eslint found some issues. Run yarn lint --fix to automatically fix problems.

Comment thread packages/eslint-config-react-native-community/index.js Outdated
@vikrantnegi
Copy link
Copy Markdown

@iRoachie You are a lifesaver. I've been struggling with this issue ever since react-native came with the eslint config.

Thanks for the fix.

Comment thread packages/eslint-config-react-native-community/index.js Outdated
Currently, @react-native-community config package extends from prettier/recommended which comes with default settings from prettier. However there are still some eslint rules in the config that either clash or duplicate the settings from prettier.

This results in eslint fixing the formatting and then prettier undoing it. This PR removes the style specific rules from eslint and place them in the prettier section.
@thymikee thymikee requested a review from elicwhite October 17, 2019 08:44
Copy link
Copy Markdown
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Copy Markdown
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Copy Markdown
Collaborator

This pull request was successfully merged by @iRoachie in e4b62bb.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Oct 28, 2019
// Prettier Plugin
'prettier/prettier': [
1,
{singleQuote: true, trailingComma: 'es5', semi: true},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iRoachie - I wonder how this section is related to the default content of .prettierrc.js:

module.exports = {
  bracketSpacing: false,
  jsxBracketSameLine: true,
  singleQuote: true,
  trailingComma: 'all',
};

Does it override and/or make it (partially) obsolete?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure exactly, what i do know is that the contents of that file doesn’t get passed onto user projects and previously you had to have a .prettierrc of your own

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay thanks for your reply. If (part of?) that file is obsolete (after updating the @react-native-community/eslint-config dependency), maybe we should update the template file as well?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's still useful for formatting a file manually, and formatting non-js files such as markdown etc.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@satya164 - Thanks for your comment! Could you give an example (or a link to one)?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prettier can format a wide-range of files such as markdown files (including code inside markdown files). The ESLint config only applies to TypeScript and JS files. Not sure what kind of example you're expecting?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay never mind, I understand the reason. Thanks!

@cpojer
Copy link
Copy Markdown
Contributor

cpojer commented Oct 29, 2019

Actually I have to revert this change because it's causing trouble with our setup at Facebook.

@friederbluemle
Copy link
Copy Markdown
Contributor

Basically, without the changes in this PR, you're required to have a .prettierrc.js with the following config as well:

module.exports = {
  bracketSpacing: false,
  jsxBracketSameLine: true,
  singleQuote: true,
  trailingComma: 'all',
};

Otherwise, you'd run into the problem as stated by @iRoachie - Is that assumption correct?

@satya164
Copy link
Copy Markdown
Contributor

You're not required to have that file. It'd just use prettier's default config.

@iRoachie
Copy link
Copy Markdown
Contributor Author

@cpojer Sigh 😔 thanks everyone for your time. Will just use another config

@friederbluemle
Copy link
Copy Markdown
Contributor

You're not required to have that file. It'd just use prettier's default config.

What I meant was that if you don't have it, you'd run into the problem described by @iRoachie

Let's try to find a solution that works - A couple of weeks ago I also tried to used @react-native-community/eslint-config and had linter conflicts related to the quotes.

@cpojer - Could you elaborate a bit on the trouble it caused?

@satya164
Copy link
Copy Markdown
Contributor

The issue that @iRoachie described was because of conflicting eslint rules being enabled, not presence of the file.

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

Labels

Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants