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

Fresh RN projects fails ESLint / Prettier by default #25478

Closed
wants to merge 2 commits into from
Closed

Fresh RN projects fails ESLint / Prettier by default #25478

wants to merge 2 commits into from

Conversation

jpdriver
Copy link
Contributor

@jpdriver jpdriver commented Jul 3, 2019

Summary

Fixes #25477

This PR adds a .prettierrc.js config file to the HelloWorld template.

eslint-config-react-native-community includes custom settings for some rules which conflict with Prettier's default settings.

Consequently, if you run eslint immediately after scaffolding a new app you get errors (see linked issue).

This PR configures Prettier to be compatible with both the existing ESLint config and the existing project template (with no code changes to the latter).

Changelog

[General] [Changed] - Added a default Prettier config for new projects

Test Plan

  • The following screenshots show the output of yarn lint before and after these changes
  • These were run immediate after running npx react-native-cli init RN060

Without these changes

  • Linting errors on new projects
  • Unfixable automatically due to conflicting rules

Screenshot 2019-07-03 at 17 44 55

Screenshot 2019-07-03 at 17 45 07

With these changes

  • Brand new projects will not produce lint errors out of the box

Screenshot 2019-07-03 at 17 48 25

@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 Jul 3, 2019
Copy link

@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.

@cpojer
Copy link
Contributor

cpojer commented Jul 4, 2019

Ideally we won't change the default and instead fix up the eslint config so that this problem doesn't happen. Could you make that change?

Copy link

@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.

@SpaghettiC0des
Copy link

Adding .prettierrc solves my problem

{
  "trailingComma": "es5",
  "tabWidth": 2,
  "singleQuote": true
}

Copy link
Contributor

@sunnylqm sunnylqm left a comment

Choose a reason for hiding this comment

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

@jpdriver Can you update the template as well?

@satya164
Copy link
Contributor

satya164 commented Jul 9, 2019

I think we should add a .prettierc.json by default to the template

@cpojer
Copy link
Contributor

cpojer commented Jul 9, 2019

Yes let's do that!

Copy link

@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.

bracketSpacing: false,
jsxBracketSameLine: true,
singleQuote: true,
trailingComma: 'es5'

Choose a reason for hiding this comment

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

comma-dangle: Missing trailing comma.

@jpdriver
Copy link
Contributor Author

jpdriver commented Jul 9, 2019

added a .prettierrc as per @satya164 's suggestion.

I did this as a .js using module.exports so it matches the corresponding default .eslintrc.js.

with a few minimal rules we're able to lint the HelloWorld template with no template changes

Copy link

@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.

Copy link

@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.

'trailingComma': 'all',
'bracketSpacing': false,
'jsxBracketSameLine': true,
}

Choose a reason for hiding this comment

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

semi: Missing semicolon.

@cpojer
Copy link
Contributor

cpojer commented Jul 10, 2019

Can you run prettier? :)

@jpdriver
Copy link
Contributor Author

@cpojer I think everything's ready and this can be landed now. Sorry I made the PR a little messy -- happy to open a fresh one if needed..

Copy link
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.

Thanks!

Copy link
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.

@keshav00001
Copy link

hi......
i am following your suggestion but i am getting same issue.

Copy link
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 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
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 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @jpdriver in 7254bab.

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 Jul 15, 2019
@RobinCsl
Copy link

RobinCsl commented Jul 15, 2019

@cpojer Would it be necessary to add prettierrc.js to this list in @react-native-community/cli too so that _prettierrc.js gets renamed to .prettierrc.js?

Results of ./cli.js init --template=file:///path/to/react-native TestProject

BEFORE patching node_modules/@react-native-community/cli/build/commands/init/editTemplate.js
Screenshot 2019-07-15 at 17 35 44

AFTER patching node_modules/@react-native-community/cli/build/commands/init/editTemplate.js by adding 'prettierrc.js' to the list UNDERSCORED_DOTFILES

Screenshot 2019-07-15 at 17 33 57

@satya164
Copy link
Contributor

@RobinCsl will be great if you can send a PR

RobinCsl added a commit to RobinCsl/cli that referenced this pull request Jul 15, 2019
In facebook/react-native@65eea9d, the prettier config for ESLint has been extracted to a .prettierrc file.

It was however not added to the React Native `template` folder and this was causing linting issues in newly generated RN apps.

In that [PR](facebook/react-native#25478) to `react-native`, a `_prettierrc.js` file was added to the `template` folder, however, a newly generated project would have the file still underscored (see facebook/react-native#25478 (comment))

This commit addresses that issue by adding `prettierrc.js` to the list of underscored files that need to be renamed to dotfiles.
thymikee pushed a commit to react-native-community/cli that referenced this pull request Jul 15, 2019
In facebook/react-native@65eea9d, the prettier config for ESLint has been extracted to a .prettierrc file.

It was however not added to the React Native `template` folder and this was causing linting issues in newly generated RN apps.

In that [PR](facebook/react-native#25478) to `react-native`, a `_prettierrc.js` file was added to the `template` folder, however, a newly generated project would have the file still underscored (see facebook/react-native#25478 (comment))

This commit addresses that issue by adding `prettierrc.js` to the list of underscored files that need to be renamed to dotfiles.
grabbou pushed a commit that referenced this pull request Aug 7, 2019
Summary:
Fixes #25477

This PR adds a `.prettierrc.js` config file to the HelloWorld template.

`eslint-config-react-native-community` includes custom settings for some rules which conflict with Prettier's default settings.

Consequently, if you run `eslint` immediately after scaffolding a new app you get errors (see linked issue).

This PR configures Prettier to be compatible with both the existing ESLint config and the existing project template (with no code changes to the latter).

## Changelog

[General] [Changed] - Added a default Prettier config for new projects
Pull Request resolved: #25478

Test Plan:
- The following screenshots show the output of `yarn lint` before and after these changes
- These were run immediate after running `npx react-native-cli init RN060`

### Without these changes

- Linting errors on new projects
- Unfixable automatically due to conflicting rules
<img width="1116" alt="Screenshot 2019-07-03 at 17 44 55" src="https://user-images.githubusercontent.com/2393035/60610078-f6b44d00-9dba-11e9-826f-1524b949e4fd.png">
<img width="1116" alt="Screenshot 2019-07-03 at 17 45 07" src="https://user-images.githubusercontent.com/2393035/60610085-fb790100-9dba-11e9-9a9c-33f4cfefd51e.png">

### With these changes

- Brand new projects will not produce lint errors out of the box
<img width="1116" alt="Screenshot 2019-07-03 at 17 48 25" src="https://user-images.githubusercontent.com/2393035/60610266-57dc2080-9dbb-11e9-8a55-fd09f3549c17.png">

Differential Revision: D16223094

Pulled By: cpojer

fbshipit-source-id: bd2c00b1fcf27b1afcad8c18b357b95a3900bdf7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Fresh RN projects fails ESLint / Prettier by default
10 participants