Skip to content

[ESLint] Fix ESLint violations in the currently ignored files for the design-system-react-native package #658

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

Open
georgewrmarshall opened this issue May 11, 2025 · 0 comments
Assignees

Comments

@georgewrmarshall
Copy link
Contributor

georgewrmarshall commented May 11, 2025

Description

Fix ESLint violations in the currently ignored files for the design-system-react-native package and remove them from the ignore list. This will improve code quality, type safety, and maintainability across the entire codebase.

Technical Details

Several files in the design-system-react-native package are currently ignored in the ESLint configuration. These files contain violations that need to be addressed, particularly in components like:

  • Avatar components (AvatarFavicon, AvatarGroup, AvatarNetwork, AvatarToken)
  • Badge components
  • Utility components like Jazzicon and Maskicon

Common violations include:

  • no-bitwise: Using bitwise operators like <<, &, and >>
  • @typescript-eslint/no-non-null-assertion: Using non-null assertion operator (!)
  • Import ordering issues
  • Type safety concerns

Approach

  1. Work through each ignored file in the design-system-react-native package, addressing its specific ESLint violations
  2. Prioritize files with fewer or simpler violations first
  3. For components with bitwise operations (like Maskicon.utilities.ts), consider:
    • Replacing with standard arithmetic operations where possible
    • Adding proper documentation where bitwise operations are necessary
    • Using ESLint exceptions with clear explanations only when absolutely needed
  4. For TypeScript non-null assertions, replace with proper null checking
  5. Remove each file from the ignore list as violations are fixed
  6. Ensure all tests continue to pass

Acceptance Criteria

  • All design-system-react-native files are removed from the ignore list in the ESLint configuration
  • All ESLint violations are fixed or have proper exceptions with explanations
  • No new ESLint warnings are introduced
  • All tests continue to pass
  • Code functionality remains unchanged

References

  • Current ESLint configuration
  • Similar violations in the react-native package (e.g., Maskicon.utilities.ts with bitwise operations and non-null assertions)
@georgewrmarshall georgewrmarshall changed the title [ESLint] Fix id-length rule in design-system-react-native package [ESLint] Fix ESLint rules for design-system-react-native package May 11, 2025
@georgewrmarshall georgewrmarshall changed the title [ESLint] Fix ESLint rules for design-system-react-native package [ESLint] Enable and Fix Disabled ESLint Rules in design-system-react-native Package May 11, 2025
@georgewrmarshall georgewrmarshall changed the title [ESLint] Enable and Fix Disabled ESLint Rules in design-system-react-native Package [ESLint] Fix ESLint violations in the currently ignored files for the design-system-react-native package May 13, 2025
@georgewrmarshall georgewrmarshall self-assigned this May 13, 2025
georgewrmarshall added a commit that referenced this issue May 14, 2025
## **Description**

This PR fixes eslint which has been broken since [Eslint v9
upgrade](#379) (3
months! 😱). It incorporates @mcmire's [work in
core](MetaMask/core#5132) to resolve the issue
and adds a script to prevent further eslint violations via the eslint
threshold script.

The solution is based on reviewing the history of Core's [eslint config
file](https://github.com/MetaMask/core/commits/main/eslint.config.mjs)
and the latest updates in main. Due to the lack of linting for over 3
months, there are many linting violations, so we have ignored files that
need fixes outside of automated fixing.

This is the first step in resolving our linting configuration. A
follow-up PR will address the ignored files in `design-system-react` and
`design-system-react-native` libraries.
- #657
- #658

### Key updates:

- Updated eslint.config.mjs to align with core
- Added @mcmire's eslint warning threshold script
- Removed individual package eslint configurations as they no longer
work for design-system-react and design-system-react-native
- Updated contributors docs
- Updated build pipeline to better work with tailwind prettier plugin
- Automated eslint fixes with yarn lint:fix

## **Related issues**

Fixes: #629

## **Manual testing steps**

1. Run `yarn lint:eslint` to verify linting now works correctly
2. Verify that the CI pipeline completes successfully with the updated
eslint configuration
3. Confirm that the eslint threshold script properly tracks and reports
violations

## **Screenshots/Recordings**

<!-- Not applicable for this PR -->

## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs)
- [x] I've completed the PR template to the best of my ability
- [x] I've included tests if applicable
- [x] I've documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I've applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md))

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
georgewrmarshall added a commit that referenced this issue May 23, 2025
## **Description**

This PR fixes linting violations in the Maskicon component across both
React and React Native versions of the design system. The changes are
focused on addressing various ESLint warnings and following best
practices to ensure code consistency.

Key improvements include:
1. Adopting proper test assertions using `toStrictEqual()` instead of
`toEqual()`
2. Proper handling of asynchronous code with `void`
3. Following consistent import ordering conventions
4. Adding appropriate ESLint comments instead of disabling rules broadly
5. Improving type safety in the test mocks
6. Removing the Maskicon components from the ESLint ignore list now that
violations are fixed

## **Related issues**

Part of: 
- #657
- #658

## **Manual testing steps**

1. Run `yarn lint` to verify linting violations are fixed
2. Run tests for the Maskicon component to ensure functionality still
works: `yarn test Maskicon`
3. Build the component to ensure it still renders correctly: `yarn
build`

## **Screenshots/Recordings**


### React Native

<img width="48" alt="Screenshot 2025-05-22 at 2 06 16 PM"
src="https://github.com/user-attachments/assets/666fa27e-127b-4f55-bba7-47c7759aafd1"
/><img width="58" alt="Screenshot 2025-05-22 at 2 08 43 PM"
src="https://github.com/user-attachments/assets/89fc7294-722d-4a59-b989-0e23c10e9fa0"
/>


### React 

<img width="446" alt="Screenshot 2025-05-22 at 2 06 46 PM"
src="https://github.com/user-attachments/assets/de17fe24-bdca-4510-a0bc-983c68271201"
/>
<img width="455" alt="Screenshot 2025-05-22 at 2 08 30 PM"
src="https://github.com/user-attachments/assets/25e93ca2-7b52-4fb8-b598-dddad320eb48"
/>

## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs)
- [x] I've completed the PR template to the best of my ability
- [x] I've included tests if applicable
- [x] I've documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I've applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants