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

fix: Eslint on Danger not matching project config #34980

Closed

Conversation

gabrieldonadel
Copy link
Collaborator

Summary

I've been noticing this for a while now, every time that someone changes a file inside rn-tester Danger comments a bunch of warnings regarding inline styles, e.g. #34567 (comment), even though that is disabled inside .eslintrc

"react-native/no-inline-styles": 0

After some investigation, I realized that the problem was that the Eslint Node.js API used by @seadub/danger-plugin-eslint was not able to locate any .eslintrc files due to the location of the directory where danger is invoked. By using process.chdir we can ensure that eslint will run from the root folder and that it will be able to find all .eslintrc files

Changelog

[Internal] [Fixed] - fix eslint config when running Danger

Test Plan

run yarn danger pr https://github.com/facebook/react-native/pull/34976

After

image

Before

image

@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, 2022
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,777,435 +4
android hermes armeabi-v7a 7,178,432 +10
android hermes x86 8,090,431 +6
android hermes x86_64 8,062,115 -5
android jsc arm64-v8a 9,636,875 -6
android jsc armeabi-v7a 8,401,168 +0
android jsc x86 9,586,058 +0
android jsc x86_64 10,179,228 +2

Base commit: a575472
Branch: main

@analysis-bot
Copy link

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

Base commit: a575472
Branch: main

Comment on lines +112 to 114
// Ensures that eslint is run from root folder and that it can find .eslintrc
process.chdir('../../');
eslint.default();
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it also work if you instead set options.cwd?

Documentation: https://eslint.org/docs/latest/developer-guide/nodejs-api

Suggested change
// Ensures that eslint is run from root folder and that it can find .eslintrc
process.chdir('../../');
eslint.default();
eslint.default({
// Ensures that eslint is run from root folder and that it can find .eslintrc
cwd: '../../',
});

Copy link
Collaborator Author

@gabrieldonadel gabrieldonadel Oct 14, 2022

Choose a reason for hiding this comment

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

Not really, that was my first approach too @yungsters but unfortunately @sgtcoolguy/danger-plugin-eslint only allows us to change the baseConfig.

https://github.com/sgtcoolguy/danger-plugin-eslint/blob/master/src/index.ts#L24

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh no… thanks for sharing. And yes, as @cipolleschi shared, thanks for fixing! I suspect this was one of the issues that blocked me from making progress on #34401. 🥰

@cipolleschi
Copy link
Contributor

Woah, Great finding @gabrieldonadel! I think this happened when we moved danger in a node package within the packages folder.

@gabrieldonadel gabrieldonadel requested review from yungsters and removed request for hramos October 14, 2022 12:07
@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.

@gabrieldonadel gabrieldonadel deleted the fix/danger-eslint branch October 14, 2022 17:06
@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @gabrieldonadel in 55b09da.

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 14, 2022
mohitcharkha pushed a commit to mohitcharkha/react-native that referenced this pull request Oct 17, 2022
Summary:
I've been noticing this for a while now, every time that someone changes a file inside rn-tester Danger comments a bunch of warnings regarding inline styles, e.g. facebook#34567 (comment), even though that is disabled inside .eslintrc https://github.com/facebook/react-native/blob/8edf4e9e3adcc85743e3c86a25ab3748b276a3da/packages/rn-tester/.eslintrc#L3

After some investigation, I realized that the problem was that the Eslint Node.js API used by [seadub/danger-plugin-eslint](https://www.npmjs.com/package/seadub/danger-plugin-eslint) was not able to locate any `.eslintrc` files due to the location of the directory where danger is invoked. By using `process.chdir` we can ensure that eslint will run from the root folder and that it will be able to find all .eslintrc files

## Changelog

[Internal] [Fixed] - fix eslint config when running Danger

Pull Request resolved: facebook#34980

Test Plan:
run `yarn danger pr https://github.com/facebook/react-native/pull/34976`

After

![image](https://user-images.githubusercontent.com/11707729/195751496-5225a32f-f4b3-4ce2-833f-ae5723f647c0.png)

Before

![image](https://user-images.githubusercontent.com/11707729/195751673-34ba87fc-ce50-4020-9688-a486e3021c4f.png)

Reviewed By: cortinico

Differential Revision: D40384300

Pulled By: yungsters

fbshipit-source-id: e68eeafc42567dc9d7297dde3709a989cc70f4e2
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
I've been noticing this for a while now, every time that someone changes a file inside rn-tester Danger comments a bunch of warnings regarding inline styles, e.g. facebook#34567 (comment), even though that is disabled inside .eslintrc https://github.com/facebook/react-native/blob/8edf4e9e3adcc85743e3c86a25ab3748b276a3da/packages/rn-tester/.eslintrc#L3

After some investigation, I realized that the problem was that the Eslint Node.js API used by [seadub/danger-plugin-eslint](https://www.npmjs.com/package/seadub/danger-plugin-eslint) was not able to locate any `.eslintrc` files due to the location of the directory where danger is invoked. By using `process.chdir` we can ensure that eslint will run from the root folder and that it will be able to find all .eslintrc files

## Changelog

[Internal] [Fixed] - fix eslint config when running Danger

Pull Request resolved: facebook#34980

Test Plan:
run `yarn danger pr https://github.com/facebook/react-native/pull/34976`

After

![image](https://user-images.githubusercontent.com/11707729/195751496-5225a32f-f4b3-4ce2-833f-ae5723f647c0.png)

Before

![image](https://user-images.githubusercontent.com/11707729/195751673-34ba87fc-ce50-4020-9688-a486e3021c4f.png)

Reviewed By: cortinico

Differential Revision: D40384300

Pulled By: yungsters

fbshipit-source-id: e68eeafc42567dc9d7297dde3709a989cc70f4e2
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.

6 participants