MetaMask / MetaMask/metamask-mobile
[Bug]: ButtonBase accepts both 'disabled' and 'isDisabled' props causing API confusion
- Dominant language
- TypeScript
- Stars
- 3k
- Forks
- 1.7k
- Avg merge
- 1d 14h
- Merged PRs (30d)
- 669
Description
## Describe the bug
The `ButtonBase` component extends `TouchableOpacityProps`, which includes a `disabled` prop. However, the component defines and uses its own `isDisabled` prop. This creates a confusing API surface where:
1. TypeScript allows passing both `disabled` (inherited from `TouchableOpacityProps`) and `isDisabled` (custom prop)
2. Only `isDisabled` is properly handled by the component logic
3. If someone passes `disabled` instead of `isDisabled`, it may not work as expected due to the explicit `disabled={isDisabled}` mapping on line 45
**Affected Component**: `app/component-library/components/Buttons/Button/foundation/ButtonBase`
**Code References**:
- `ButtonBase.types.ts:12` - Interface extends `TouchableOpacityProps`
- `ButtonBase.tsx:33` - `isDisabled` prop is destructured
- `ButtonBase.tsx:45` - `disabled={isDisabled}` explicitly maps the custom prop
- `ButtonBase.tsx:51` - `{...props}` spreads remaining props, potentially including inherited `disabled`
## Expected behavior
The component should have a clear, unambiguous API that only accepts `isDisabled` and explicitly omits the `disabled` prop from `TouchableOpacityProps` to prevent confusion and potential bugs.
## Steps to reproduce
1. Open `app/component-library/components/Buttons/Button/foundation/ButtonBase/ButtonBase.types.ts`
2. Note that `ButtonBaseProps extends TouchableOpacityProps` (line 12)
3. See that both `disabled` (from TouchableOpacity) and `isDisabled` (custom) are technically valid props
4. Check `ButtonBase.tsx:45` where only `isDisabled` is used: `disabled={isDisabled}`
5. Attempt to use `` - TypeScript won't complain but it may not work correctly
## Proposed Solution
Explicitly omit the `disabled` prop from `TouchableOpacityProps`:
```typescript
export interface ButtonBaseProps extends Omit {
// ... existing props
isDisabled?: boolean;
}
```
This ensures:
- Only `isDisabled` is accepted by the component API
- TypeScript will error if someone tries to use `disabled`
- The API is clear and unambiguous
## Where was this bug found?
Internal code review
## Additional context
This is a technical debt issue that affects API clarity and could lead to bugs if developers mistakenly use `disabled` instead of `isDisabled`. While the component is marked as deprecated in favor of `@metamask/design-system-react-native`, it's still used in the codebase and should have a clear API until fully migrated.
Contributor guide
Assessment
This issue has not been assessed yet.