Skip to content

fix: Eslint on Danger not matching project config - #34980

Closed
gabrieldonadel wants to merge 1 commit into
react:mainfrom
gabrieldonadel:fix/danger-eslint
Closed

fix: Eslint on Danger not matching project config#34980
gabrieldonadel wants to merge 1 commit into
react:mainfrom
gabrieldonadel:fix/danger-eslint

Conversation

@gabrieldonadel

Copy link
Copy Markdown
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 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 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-botfacebook-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
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a7,777,435+4
androidhermesarmeabi-v7a7,178,432+10
androidhermesx868,090,431+6
androidhermesx86_648,062,115-5
androidjscarm64-v8a9,636,875-6
androidjscarmeabi-v7a8,401,168+0
androidjscx869,586,058+0
androidjscx86_6410,179,228+2

Base commit: a575472
Branch: main

@analysis-bot

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
ios-universaln/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
Copy Markdown
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: '../../',
});

@gabrieldonadelgabrieldonadelOct 14, 2022

Copy link
Copy Markdown
CollaboratorAuthor

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
Copy Markdown
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
Copy Markdown
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 hramosOctober 14, 2022 12:07
@facebook-github-bot

Copy link
Copy Markdown
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
Copy Markdown
Collaborator

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

When will my fix make it into a release? | Upcoming Releases

@react-native-botreact-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. react#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: react#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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BugCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@gabrieldonadel@analysis-bot@cipolleschi@facebook-github-bot@react-native-bot@yungsters