Functional component for a modern approach#647
Functional component for a modern approach#647finnholland wants to merge 5 commits intohyochan:mainfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the existing class-based AudioRecorderPlayer to extract shared types into types.ts and introduce a functional-hook-based implementation (AudioRecorderPlayerFC), also updating package scripts and dependencies.
- Moved all enums, interfaces, and utility (
pad) into a standalonetypes.ts - Updated
index.tsto import shared types, export both class and functional versions, and added new status getters - Added a new
index.fc.tsfile providing a hook-style API, and adjusted lint scripts and dependencies inpackage.json
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| types.ts | New shared enums, interfaces, and pad helper export |
| index.ts | Refactored to import types, export functional API, and added status flags |
| index.fc.ts | New functional component using React Hooks |
| package.json | Broadened lint targets, added lint:fix, and included react dependency |
Comments suppressed due to low confidence (3)
index.fc.ts:26
- [nitpick]
mmssandmmssssare not descriptive. Consider renaming to more explicit names likeformatMinutesSecondsandformatMinutesSecondsMilliseconds.
const mmss = (secs: number): string => {
package.json:45
- Adding
reactas a direct dependency may cause version conflicts for library consumers. Consider moving it topeerDependencies.
"react": "^19.0.0",
index.fc.ts:1
- The new functional implementation lacks accompanying tests. Please add unit tests to cover core behaviors like start/stop recorder/player and
getStatus().
import {useState} from 'react';
| private _isStopped: boolean; | ||
| private _isFinished: boolean; |
There was a problem hiding this comment.
The newly added _isStopped and _isFinished fields are never initialized in the constructor, so they default to undefined. Initialize them (e.g., this._isStopped = false; this._isFinished = false;) to avoid unexpected values in getStatus().
| private _isStopped: boolean; | |
| private _isFinished: boolean; | |
| private _isStopped: boolean = false; | |
| private _isFinished: boolean = false; |
| useState<EmitterSubscription | null>(null); | ||
| const [playerSubscription, setPlayerSubscription] = | ||
| useState<EmitterSubscription | null>(null); | ||
| let _playerCallback: ((event: PlayBackType) => void) | null; |
There was a problem hiding this comment.
Using a local let for _playerCallback will reset on each render. Consider using a useRef to persist the callback across re-renders.
| let _playerCallback: ((event: PlayBackType) => void) | null; | |
| const _playerCallback = useRef<((event: PlayBackType) => void) | null>(null); |
| * Set listener from native module for player. | ||
| * @returns {callBack((e: PlayBackType): void)} | ||
| */ | ||
| const addPlayBackListener = ( |
There was a problem hiding this comment.
addPlayBackListener only sets a callback but never subscribes to the native event emitter. You should mirror the subscription logic from startPlayer to ensure events are actually received.
| }; | ||
|
|
||
| /** | ||
| * Set listerner from native module for recorder. |
There was a problem hiding this comment.
There's a typo: listerner should be spelled listener.
| * Set listerner from native module for recorder. | |
| * Set listener from native module for recorder. |
11feb3a to
14b6492
Compare
Creating a more modern react native module to utilise
useStateandconststatements.index.tsto better service modern react native compared to class based.types.tsso they can be shared across the two files (this is optional but nicer to manage imo)Wasn't sure on naming conventions so feel free to veto those.
I have tested using my example but will confirm with the example in the package later today.