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

Separate ESLint and Prettier #3373

Merged
merged 5 commits into from
Apr 2, 2024
Merged

Separate ESLint and Prettier #3373

merged 5 commits into from
Apr 2, 2024

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Apr 2, 2024

Our current setup is running Prettier as a part of ESLint run because of facebook/react-native#43756. This wasn't super intentional and is just a consequence of us using the default configs which have this arcane setting turned on.

The recommended modern approach is to treat Prettier and ESLint as separate unrelated tools.
This has a few benefits:

  • Editors that support Prettier will no longer run Prettier twice on every save (currently they do because we lint in addition to formatting, but linting also includes formatting).
  • Formatting nits will no longer show up in IDEs as errors or warnings (currently they do because, well, for the same reason).
  • This may also prevent some cases where lint has to run more than once (due to cycles).

See Prettier's Integrating with Linters and Josh Goldberg's blog post for more background.

Okay, so what are we doing?

  • First, I get rid of eslint-plugin-prettier by explicitly disabling the prettier/prettier rule. We'll be able to remove this special case when Replace outdated Prettier settings with recommended ones facebook/react-native#43756 ships.
  • Next, I add explicit yarn prettier --check . to CI.
  • I tweak the .eslintignore to select the stuff we want (code in src) and exclude the rest. Previously it was kind of selectively ignoring a bunch of stuff but it was also being called outside just for src so there was no source of truth.
  • I extend the lint staged commands to run Prettier after ESLint. Note I removed the yarn indirection there to cut down time on node init. Otherwise we have a chain of Node processes which is slower than we want.
  • Finally, added --cache to both commands as suggested by Christoph Nakazawa, it seems to be working decent in my testing.

If I didn't mess it up, this should give us roughly same behavior as before but faster and less annoying.

Copy link

github-actions bot commented Apr 2, 2024

The Pull Request introduced fingerprint changes against the base commit: 0ff7e71

Fingerprint diff
[
  {
    "type": "file",
    "filePath": ".gitignore",
    "reasons": [
      "bareGitIgnore"
    ],
    "hash": "9a4ece6359c958ac13d9d9d770a2fc4fb00c99a0"
  },
  {
    "type": "file",
    "filePath": "package.json",
    "reasons": [
      "expoConfigPlugins"
    ],
    "hash": "7128c1cabb8455b0deca33e941f2efae77cc1185"
  },
  {
    "type": "contents",
    "id": "packageJson:scripts",
    "contents": "{\"prepare\":\"is-ci || husky install\",\"postinstall\":\"patch-package && yarn intl:compile\",\"prebuild\":\"expo prebuild --clean\",\"android\":\"expo run:android\",\"ios\":\"expo run:ios\",\"web\":\"expo start --web\",\"use-build-number\":\"./scripts/useBuildNumberEnv.sh\",\"build-web\":\"expo export:web && node ./scripts/post-web-build.js && cp -v ./web-build/static/js/*.* ./bskyweb/static/js/\",\"build-all\":\"yarn intl:build && yarn use-build-number eas build --platform all\",\"build-ios\":\"yarn use-build-number eas build -p ios\",\"build-android\":\"yarn use-build-number eas build -p android\",\"build\":\"yarn use-build-number eas build\",\"start\":\"expo start --dev-client\",\"start:prod\":\"expo start --dev-client --no-dev --minify\",\"clean-cache\":\"rm -rf node_modules/.cache/babel-loader/*\",\"test\":\"NODE_ENV=test jest --forceExit --testTimeout=20000 --bail\",\"test-watch\":\"NODE_ENV=test jest --watchAll\",\"test-ci\":\"NODE_ENV=test jest --ci --forceExit --reporters=default --reporters=jest-junit\",\"test-coverage\":\"NODE_ENV=test jest --coverage\",\"lint\":\"yarn eslint --cache --ext .js,.jsx,.ts,.tsx src\",\"typecheck\":\"tsc --project ./tsconfig.check.json\",\"e2e:mock-server\":\"./jest/dev-infra/with-test-redis-and-db.sh ts-node --project tsconfig.e2e.json __e2e__/mock-server.ts\",\"e2e:metro\":\"NODE_ENV=test RN_SRC_EXT=e2e.ts,e2e.tsx expo run:ios\",\"e2e:build\":\"NODE_ENV=test detox build -c ios.sim.debug\",\"e2e:run\":\"NODE_ENV=test detox test --configuration ios.sim.debug --take-screenshots all\",\"perf:test\":\"NODE_ENV=test maestro test\",\"perf:test:run\":\"NODE_ENV=test maestro test __e2e__/maestro/scroll.yaml\",\"perf:test:measure\":\"NODE_ENV=test flashlight test --bundleId xyz.blueskyweb.app --testCommand 'yarn perf:test' --duration 150000 --resultsFilePath .perf/results.json\",\"perf:test:results\":\"NODE_ENV=test flashlight report .perf/results.json\",\"perf:measure\":\"NODE_ENV=test flashlight measure\",\"intl:build\":\"yarn intl:extract && yarn intl:compile\",\"intl:check\":\"yarn intl:extract && git diff-index -G'(^[^\\\\*# /])|(^#\\\\w)|(^\\\\s+[^\\\\*#/])' HEAD || (echo '\\n⚠️ i18n detected un-extracted translations\\n' && exit 1)\",\"intl:extract\":\"lingui extract\",\"intl:compile\":\"lingui compile\",\"nuke\":\"rm -rf ./node_modules && rm -rf ./ios && rm -rf ./android\",\"update-extensions\":\"bash scripts/updateExtensions.sh\",\"export\":\"npx expo export\",\"make-deploy-bundle\":\"bash scripts/bundleUpdate.sh\"}",
    "reasons": [
      "packageJson:scripts"
    ],
    "hash": "c18ddef91d77aa574fc23064280b0ec5106e2e02"
  }
]

Generated by PR labeler 🤖

@haileyok
Copy link
Contributor

haileyok commented Apr 2, 2024

Amazing! Pulled this down and I'm getting the expected behavior in Webstorm now. Prettier only runs on save if I explicitly tell Webstorm to do so, rather than it running all the time w/ eslint. Seriously appreciate you looking into this, it was killing performance for me!

Will be curious to see over time if the --cache flag improves performance as well. Never knew about this one. Code in ./modules/*/src is also linted correctly so .eslintignore looks solid.

@gaearon
Copy link
Collaborator Author

gaearon commented Apr 2, 2024

OK let's give this a go.

@gaearon
Copy link
Collaborator Author

gaearon commented Apr 2, 2024

#codingonaplane

@gaearon gaearon merged commit a45da17 into main Apr 2, 2024
5 checks passed
@gaearon gaearon deleted the separate-eslint-prettier branch April 2, 2024 05:58
ahuth added a commit to ahuth/andrew-stack that referenced this pull request Apr 2, 2024
@estrattonbailey
Copy link
Member

Nice one!

facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Apr 4, 2024
Summary:
The React Native ESLint preset currently endorses the Prettier integration that is [explicitly recommended against by Pretier itself](https://prettier.io/docs/en/integrating-with-linters). Notice the difference between these two packages:

- `eslint-config-prettier` is the config that turns off all formatting rules. It's **recommended by Prettier** to be used together with Prettier. You'd still use Prettier itself to actually do the formatting.
- `eslint-plugin-prettier` is a legacy plugin developed a long time ago and that predates most modern Prettier integrations. It runs Prettier as if it were an ESLint rule, applies formatting on `--fix`, and **is not recommended**.

Unfortunately, RN uses the latter one (and always has).

This PR removes `eslint-plugin-prettier` and instead enables `eslint-config-prettier`, as recommended by Prettier.

As a consequence, you'll no longer see squiggly lines in your editor for stuff that isn't actually errors:

<img width="558" alt="Screenshot 2024-04-01 at 20 00 50" src="https://github.com/facebook/react-native/assets/810438/91ae2cec-a9ef-4205-a9ce-6ab858785ed2">

As another consequence, **you'll have to set up your own Prettier step in your pipeline**.

For example, if your precommit hook only contained `eslint --fix`, you'll now also need to run `prettier --write` there as well. Similarly, if you want Prettier to fail CI, you'd need to find where you call `eslint` and also do `prettier --check` there.

Here's an example for how to do it: bluesky-social/social-app#3373

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[GENERAL] [BREAKING] - RN ESLint config no longer runs Prettier during ESLint

Pull Request resolved: #43756

Test Plan:
Tested locally, verified formatting changes no longer get flagged as violations by the RN config.

<img width="470" alt="Screenshot 2024-04-01 at 20 33 55" src="https://github.com/facebook/react-native/assets/810438/515db971-18bc-4625-bb6d-b9d072692923">

Reviewed By: motiz88

Differential Revision: D55643699

Pulled By: yungsters

fbshipit-source-id: 97df774275922086f0356ac857d6425713184e39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants