-
Notifications
You must be signed in to change notification settings - Fork 231
Functional component for a modern approach #647
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
base: main
Are you sure you want to change the base?
Conversation
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.
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.ts
to import shared types, export both class and functional versions, and added new status getters - Added a new
index.fc.ts
file 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]
mmss
andmmssss
are not descriptive. Consider renaming to more explicit names likeformatMinutesSeconds
andformatMinutesSecondsMilliseconds
.
const mmss = (secs: number): string => {
package.json:45
- Adding
react
as 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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; |
Copilot uses AI. Check for mistakes.
useState<EmitterSubscription | null>(null); | ||
const [playerSubscription, setPlayerSubscription] = | ||
useState<EmitterSubscription | null>(null); | ||
let _playerCallback: ((event: PlayBackType) => void) | null; |
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.
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); |
Copilot uses AI. Check for mistakes.
* Set listener from native module for player. | ||
* @returns {callBack((e: PlayBackType): void)} | ||
*/ | ||
const addPlayBackListener = ( |
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.
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.
Copilot uses AI. Check for mistakes.
}; | ||
|
||
/** | ||
* Set listerner from native module for recorder. |
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.
There's a typo: listerner
should be spelled listener
.
* Set listerner from native module for recorder. | |
* Set listener from native module for recorder. |
Copilot uses AI. Check for mistakes.
Creating a more modern react native module to utilise
useState
andconst
statements.index.ts
to better service modern react native compared to class based.types.ts
so 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.