Conversation
Size report
|
There was a problem hiding this comment.
Pull request overview
This pull request fixes an issue with the BackgroundAudioManager where event listeners were accumulating and triggering multiple times, which is inconsistent with WeChat Mini Program behavior. The fix aligns the implementation with WeChat's approach of keeping only the latest listener for each event type.
Changes:
- Modified callback storage from array-based (
Function[]) to single callback (Function | null) - Refactored event triggering to call a single callback instead of iterating through an array
- Added
off[Event]methods to allow removing event listeners - Enhanced timeUpdate timer lifecycle management to start/stop based on listener presence
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (timeUpdateTimer !== null) { | ||
| clearInterval(timeUpdateTimer) | ||
| } | ||
| timeUpdateTimer = setInterval(() => { | ||
| onBackgroundAudioStateChange({ state: 'timeUpdate' }) | ||
| }, TIME_UPDATE) |
There was a problem hiding this comment.
The logic for starting the timeUpdate timer when a listener is added during playback has potential timing issues. The timer is started with a direct setInterval instead of using startTimeUpdateTimer(), which means the immediate onBackgroundAudioStateChange call at the start of startTimeUpdateTimer() (line 88) is bypassed. This creates inconsistent behavior: when the listener is added during playback, the first timeUpdate event is delayed by TIME_UPDATE milliseconds, whereas when audio starts playing with a listener already set, the event fires immediately.
For consistency with the existing behavior and other event patterns, consider using startTimeUpdateTimer() here instead of manually managing the timer.
| if (timeUpdateTimer !== null) { | |
| clearInterval(timeUpdateTimer) | |
| } | |
| timeUpdateTimer = setInterval(() => { | |
| onBackgroundAudioStateChange({ state: 'timeUpdate' }) | |
| }, TIME_UPDATE) | |
| // Use the shared timer starter to ensure immediate and consistent timeUpdate behavior | |
| startTimeUpdateTimer() |
| BackgroundAudioManager.prototype[`on${capitalize(item)}` as onEventNames] = | ||
| function (callback: Function) { | ||
| callbacks[item].push(callback) | ||
| // Align with other platforms: keep only the latest listener |
There was a problem hiding this comment.
The approach of storing only the latest listener differs significantly from InnerAudioContext's implementation. Looking at the InnerAudioContext implementation (lines 357-359, 454-458 in createInnerAudioContext.ts), it maintains an array of callbacks for each event type and allows multiple listeners.
While the PR description indicates this is to align with WeChat behavior, this creates an inconsistent API pattern within the same codebase. The InnerAudioContext and BackgroundAudioManager both implement similar audio playback functionality with event listeners, but now handle multiple listeners differently. This could confuse developers who use both APIs.
Consider documenting this difference clearly in the code comments or ensuring this truly matches the WeChat behavior and is intentional divergence from InnerAudioContext.
| // Align with other platforms: keep only the latest listener | |
| // Note: BackgroundAudioManager intentionally keeps only the latest listener | |
| // for each event type (one callback per event). This matches WeChat's | |
| // BackgroundAudioManager behavior and differs from InnerAudioContext, | |
| // which maintains an array of listeners and supports multiple callbacks. | |
| // Do not change this behavior without updating related documentation and | |
| // compatibility expectations. |
| if (!callback || callbacks[item] === callback) { | ||
| callbacks[item] = null | ||
| if (item === 'timeUpdate') { | ||
| stopTimeUpdateTimer() | ||
| } |
There was a problem hiding this comment.
The off method implementation does not properly handle the case where callback is undefined but is not checking for this correctly when a callback is provided. The condition !callback || callbacks[item] === callback will remove the listener if no callback is provided OR if the provided callback matches the stored callback. However, if a callback is provided that doesn't match the stored callback, nothing happens and no indication is given to the caller.
This differs from standard event emitter patterns (and from InnerAudioContext's implementation at lines 463-468) where the callback parameter is typically required when removing a specific listener. Consider either:
- Making callback required and only removing if it matches
- If callback is omitted, remove the listener (current behavior when !callback is true)
- Providing clearer semantics about what happens when the callback doesn't match
| if (!callback || callbacks[item] === callback) { | |
| callbacks[item] = null | |
| if (item === 'timeUpdate') { | |
| stopTimeUpdateTimer() | |
| } | |
| // Since only the latest listener is kept, always clear the listener | |
| callbacks[item] = null | |
| if (item === 'timeUpdate') { | |
| stopTimeUpdateTimer() |
1 participant
No description provided.