Skip to content

Conversation

yungsters
Copy link
Contributor

Summary

Changes the React Native base ESLint configuration to consume @react-native-community/eslint-config as a yarn workspace, so that any dependencies of @react-native-community/eslint-config can also be resolved from the root directory of react-native.

Previously, ~/.eslintrc.js extended @react-native-community/eslint-config using a relative file path. This is problematic because if any dependencies (notably, optional peer dependencies such as some of the TypeScript dependencies) are not already installed at the root directory of react-native, running ESLint could fail to resolve any required dependencies. In other words, there was an implicit dependency that react-native/yarn.lock would also contain any dependencies required by react-native/packages/eslint-config-react-native-community/yarn.lock.

With this change, running yarn from the root directory of react-native will also install any dependencies of @react-native-community/eslint-config, and it will also symlink react-native/node_modules/@react-native-community/eslint-config to ../../packages/eslint-config-react-native-community (meaning any local changes to the config will still be reflected during active development).

Changelog

[Internal]

Test Plan

Successfully install dependencies and run ESLint.

$ cd react-native
$ yarn
$ yarn lint

Successfully run Danger on #34401 (which would previously fail because @typescript-eslint/eslint-plugin could not be resolved from the root directory of react-native):

$ cd react-native/bots
$ yarn
$ node ./node_modules/.bin/danger pr https://github.com/facebook/react-native/pull/34401

@yungsters yungsters requested a review from hramos as a code owner August 15, 2022 16:54
@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 Aug 15, 2022
@github-actions
Copy link

github-actions bot commented Aug 15, 2022

Warnings
⚠️ 🔒 package.json - Changes were made to package.json. This will require a manual import by a Facebook employee.

Generated by 🚫 dangerJS against cc8e243

@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Aug 15, 2022
"test-ios": "./scripts/objc-test.sh test"
},
"workspaces": [
"packages/!(eslint-config-react-native-community)",
Copy link
Contributor

Choose a reason for hiding this comment

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

Love this <3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Less special cases!

It's always a little bit scary because someone at some point in time thought this was a good idea. I could not find any reason why this was originally done, though, so… move fast and break things, I suppose. 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, actually… I found the reason in 3967635:

Note: We do not use eslint-config-react-native-community internally and it pulls in a lot of packages we don't need. It is part of the React Native repo because it is used in the RN app template but we may want to choose to move it out into a separate repo at some point.

Yeah… that's fine. We want these TypeScript dependencies installed now.

@facebook-github-bot
Copy link
Contributor

@yungsters has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@analysis-bot
Copy link

analysis-bot commented Aug 15, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,610,287 -132
android hermes armeabi-v7a 7,025,045 -137
android hermes x86 7,910,175 -141
android hermes x86_64 7,883,828 -136
android jsc arm64-v8a 9,489,076 -41
android jsc armeabi-v7a 8,266,718 -36
android jsc x86 9,426,549 -39
android jsc x86_64 10,019,314 -36

Base commit: ca1c3f9
Branch: main

@analysis-bot
Copy link

analysis-bot commented Aug 15, 2022

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: ca1c3f9
Branch: main

package.json Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, this is wrong. Due to how our repository is set up right now (to work with our internal codebase), this only has to be declared in react-native/repo-config/package.json (and it is already present there).

root: true,

extends: ['./packages/eslint-config-react-native-community/index.js'],
extends: ['@react-native-community'],
Copy link
Member

Choose a reason for hiding this comment

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

🎉

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @yungsters in be8fe7a.

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 Aug 16, 2022
roryabraham pushed a commit to Expensify/react-native that referenced this pull request Aug 17, 2022
…facebook#34423)

Summary:
Changes the React Native base ESLint configuration to consume `react-native-community/eslint-config` as a [yarn workspace](https://classic.yarnpkg.com/lang/en/docs/workspaces/), so that any dependencies of `react-native-community/eslint-config` can also be resolved from the root directory of `react-native`.

Previously, `~/.eslintrc.js` extended `react-native-community/eslint-config` using a relative file path. This is problematic because if any dependencies (notably, optional peer dependencies such as some of the TypeScript dependencies) are not already installed at the root directory of `react-native`, running ESLint could fail to resolve any required dependencies. In other words, there was an implicit dependency that `react-native/yarn.lock` would also contain any dependencies required by `react-native/packages/eslint-config-react-native-community/yarn.lock`.

With this change, running `yarn` from the root directory of `react-native` will also install any dependencies of `react-native-community/eslint-config`, and it will also symlink `react-native/node_modules/react-native-community/eslint-config` to `../../packages/eslint-config-react-native-community` (meaning any local changes to the config will still be reflected during active development).

## Changelog

[Internal]

Pull Request resolved: facebook#34423

Test Plan:
Successfully install dependencies and run ESLint.

```
$ cd react-native
$ yarn
$ yarn lint
```

Successfully run Danger on facebook#34401 (which would previously fail because `typescript-eslint/eslint-plugin` could not be resolved from the root directory of `react-native`):

```
$ cd react-native/bots
$ yarn
$ node ./node_modules/.bin/danger pr facebook#34401
```

Reviewed By: NickGerleman

Differential Revision: D38710285

Pulled By: yungsters

fbshipit-source-id: a06ceea0884a90be60f6f5db9a5d42be52a951d5
roryabraham pushed a commit to Expensify/react-native that referenced this pull request Aug 17, 2022
…facebook#34423)

Summary:
Changes the React Native base ESLint configuration to consume `react-native-community/eslint-config` as a [yarn workspace](https://classic.yarnpkg.com/lang/en/docs/workspaces/), so that any dependencies of `react-native-community/eslint-config` can also be resolved from the root directory of `react-native`.

Previously, `~/.eslintrc.js` extended `react-native-community/eslint-config` using a relative file path. This is problematic because if any dependencies (notably, optional peer dependencies such as some of the TypeScript dependencies) are not already installed at the root directory of `react-native`, running ESLint could fail to resolve any required dependencies. In other words, there was an implicit dependency that `react-native/yarn.lock` would also contain any dependencies required by `react-native/packages/eslint-config-react-native-community/yarn.lock`.

With this change, running `yarn` from the root directory of `react-native` will also install any dependencies of `react-native-community/eslint-config`, and it will also symlink `react-native/node_modules/react-native-community/eslint-config` to `../../packages/eslint-config-react-native-community` (meaning any local changes to the config will still be reflected during active development).

## Changelog

[Internal]

Pull Request resolved: facebook#34423

Test Plan:
Successfully install dependencies and run ESLint.

```
$ cd react-native
$ yarn
$ yarn lint
```

Successfully run Danger on facebook#34401 (which would previously fail because `typescript-eslint/eslint-plugin` could not be resolved from the root directory of `react-native`):

```
$ cd react-native/bots
$ yarn
$ node ./node_modules/.bin/danger pr facebook#34401
```

Reviewed By: NickGerleman

Differential Revision: D38710285

Pulled By: yungsters

fbshipit-source-id: a06ceea0884a90be60f6f5db9a5d42be52a951d5
facebook-github-bot pushed a commit that referenced this pull request Nov 4, 2022
Summary:
Basically since this change #34423 everything worked ok because in the main branch where the monorepo has the workspace section and everything, `react-native-community/eslint-config` was getting pulled in as a node_module anyway - but when moving to a stable branch and wanting to do a release, as we are now with 0.71, because of what the final package.json looks like for react-native and [the modifications that it goes through](f0054e1#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519) (ex. the `workspace` section disappears), the fact that it's not directly listed as dependency makes [the CI fails](https://app.circleci.com/pipelines/github/facebook/react-native/17124/workflows/54a4162d-f466-4eab-94ba-ec9fe77e2ecf/jobs/339643) with `Error: Failed to load config "react-native-community" to extend from.`

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[General] [Fixed] - add explicitly eslint config to dependencies

Pull Request resolved: #35192

Test Plan: In 0.71, add the deps to see the error disappear (it gets replaced by a different one though, looking into that)

Reviewed By: jacdebug, cortinico

Differential Revision: D40988407

Pulled By: cipolleschi

fbshipit-source-id: 38f433a0b4b47a7cb61b59e887459d11182a5b4b
cipolleschi pushed a commit that referenced this pull request Nov 4, 2022
Summary:
Basically since this change #34423 everything worked ok because in the main branch where the monorepo has the workspace section and everything, `react-native-community/eslint-config` was getting pulled in as a node_module anyway - but when moving to a stable branch and wanting to do a release, as we are now with 0.71, because of what the final package.json looks like for react-native and [the modifications that it goes through](f0054e1#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519) (ex. the `workspace` section disappears), the fact that it's not directly listed as dependency makes [the CI fails](https://app.circleci.com/pipelines/github/facebook/react-native/17124/workflows/54a4162d-f466-4eab-94ba-ec9fe77e2ecf/jobs/339643) with `Error: Failed to load config "react-native-community" to extend from.`

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[General] [Fixed] - add explicitly eslint config to dependencies

Pull Request resolved: #35192

Test Plan: In 0.71, add the deps to see the error disappear (it gets replaced by a different one though, looking into that)

Reviewed By: jacdebug, cortinico

Differential Revision: D40988407

Pulled By: cipolleschi

fbshipit-source-id: 38f433a0b4b47a7cb61b59e887459d11182a5b4b
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
)

Summary:
Basically since this change facebook#34423 everything worked ok because in the main branch where the monorepo has the workspace section and everything, `react-native-community/eslint-config` was getting pulled in as a node_module anyway - but when moving to a stable branch and wanting to do a release, as we are now with 0.71, because of what the final package.json looks like for react-native and [the modifications that it goes through](facebook@f0054e1#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519) (ex. the `workspace` section disappears), the fact that it's not directly listed as dependency makes [the CI fails](https://app.circleci.com/pipelines/github/facebook/react-native/17124/workflows/54a4162d-f466-4eab-94ba-ec9fe77e2ecf/jobs/339643) with `Error: Failed to load config "react-native-community" to extend from.`

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[General] [Fixed] - add explicitly eslint config to dependencies

Pull Request resolved: facebook#35192

Test Plan: In 0.71, add the deps to see the error disappear (it gets replaced by a different one though, looking into that)

Reviewed By: jacdebug, cortinico

Differential Revision: D40988407

Pulled By: cipolleschi

fbshipit-source-id: 38f433a0b4b47a7cb61b59e887459d11182a5b4b
@yungsters yungsters deleted the eslint branch July 11, 2023 00:23
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. p: Facebook Partner: Facebook Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants