-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
Conversation
There was a problem hiding this 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. Runyarn lint --fix
to automatically fix problems.
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? |
There was a problem hiding this 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. Runyarn lint --fix
to automatically fix problems.
Adding {
"trailingComma": "es5",
"tabWidth": 2,
"singleQuote": true
} |
There was a problem hiding this 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?
I think we should add a |
Yes let's do that! |
There was a problem hiding this 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. Runyarn lint --fix
to automatically fix problems.
template/_prettierrc.js
Outdated
bracketSpacing: false, | ||
jsxBracketSameLine: true, | ||
singleQuote: true, | ||
trailingComma: 'es5' |
There was a problem hiding this comment.
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.
added a I did this as a with a few minimal rules we're able to lint the HelloWorld template with no template changes ✨ |
There was a problem hiding this 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. Runyarn lint --fix
to automatically fix problems.
There was a problem hiding this 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. Runyarn lint --fix
to automatically fix problems.
template/_prettierrc.js
Outdated
'trailingComma': 'all', | ||
'bracketSpacing': false, | ||
'jsxBracketSameLine': true, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
semi: Missing semicolon.
Can you run prettier? :) |
@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.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this 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.
hi...... |
There was a problem hiding this 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.
There was a problem hiding this 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.
This pull request was successfully merged by @jpdriver in 7254bab. When will my fix make it into a release? | Upcoming Releases |
@cpojer Would it be necessary to add Results of
|
@RobinCsl will be great if you can send a PR |
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.
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.
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
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
yarn lint
before and after these changesnpx react-native-cli init RN060
Without these changes
With these changes