-
Notifications
You must be signed in to change notification settings - Fork 734
Infra/eslint v2 #1193
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
Infra/eslint v2 #1193
Conversation
* master: (77 commits) Button - refactor into smaller files (#1168) replace @react-native-community/picker with @react-native-picker/picker (#1063) Update generate types fix: catch undefined StatusBarManager (#1189) prop value shape deprecation lint rule (#1094) Infra/expo app fixes (#1166) Safe require blur-view dependency in Card (#1179) Update our stars count Update expo snack link with only supported platforms Export TabBarProps and fix BadgeScreen typescript errors Disable flipper Avoid passing overlayContent in CardSecion (#1187) Fix Carousel typings updating published version no-direct-import eslint rule - adding `applyAutofix` flag (#1183) Feat/image demo broken (#1173) Remove addItems from screen with fewItems Add vertical scroll to Carousel component (#1175) Fix Carousel gif Update Carousel gifs ... # Conflicts: # eslint-rules/lib/rules/typography-deprecation.js # eslint-rules/package.json # eslint-rules/tests/lib/rules/typography-deprecation.js # eslint-rules/tests/typography_deprecation.json
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.
Looks awesome!
I left you some comments.
For the next time - consider dividing big PRs as this one into smaller PRs, just to make it easier to review
eslint-rules/lib/utils/debugUtils.js
Outdated
const cleanObject = {}; | ||
Object.keys(object).forEach(key => { | ||
const value = object[key]; | ||
if (value && typeof value === 'object') { |
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.
I think you don't have to check if the value exists before the comparison
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.
I think you are right, but TBH it's a copy-pasted function and I don't want to change it without testing, I'll add a TODO, and hopefully someone will test it :)
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.
typeof undefined
is undefined
, so you can safely remove it :)
Description
Handling more types of imports\requires (for instance importing *).
Handling renamed components.
Our fix is not working on all the examples.
Refactor to have more and simpler to understand utils.
It's a big one, don't get too stressed over it. Try to see what you can use later from your new rule point-of-view (you'll need to edit it and improve using these utils).
Perhaps try to see if I missed something in the tests etc.
Changelog
Big refactor, support more import\require types (for instance importing *), handling renamed components, improve fix logic.