Skip to content

Conversation

@dprevost-LMI
Copy link
Contributor

@dprevost-LMI dprevost-LMI commented Oct 5, 2024

Some leftover code fixing some other leaking promises + other code refactoring

Highlights:

  • Keeping promises inside AudioWaveformModule helps to ensure promises are resolved correctly
  • Moving promises out of AudioPlayer for a leaner class and not "react-native" tainted
  • Doing try-catch so the app does not crash when having an exception

Bonus:

  • I added a feature in the example to list recorded audio after the app reboot!
  • I removed the deprecation of onPlayerStateChanged in favour of onPlaybackStateChanged
  • I reviewed the weird handling of onError and simplified it
  • In JS, I removed a few Promise.resolve and Promise.reject combined with try-catch that are useless and can be simplified by returning the value directly or throwing an error since the method is already async
  • I fixed a case where the promise leaks when the path in the preparePlayer is wrong. If we play a file with an invalid path, the player never gets ready and gets stuck idle. Since player.prepare() should move it out of idle, if it stays in idle, there is a problem, and the promise is rejected.
  • I set props of IStartPlayer mandatory else; I test it, and passing no value crashes the android, so there are no optional
  • I removed the await preparePlayerForPath(); inside the onDidFinishPlayingAudio since this one is circular, where you trigger the player's end but recreate it immediately. Also, after removal, it still works
  • Code simplification by using standard functions to validate the player key string and the audio player in the hash map

Let me know what you think!

Note: I may need to remove my logger logPromise tracking the leaking promises before you merge!

@dprevost-LMI dprevost-LMI changed the title Fix multiple leaking promises. + bonuses Fix multiple leaking promises + bonuses Oct 5, 2024
@dprevost-LMI dprevost-LMI force-pushed the develop-fix-leaking-promises branch from 12ba2e9 to bcc8999 Compare October 5, 2024 23:33
@dprevost-LMI dprevost-LMI marked this pull request as ready for review October 6, 2024 16:03
@dprevost-LMI dprevost-LMI force-pushed the develop-fix-leaking-promises branch from d737853 to 49ad08a Compare October 6, 2024 16:08
@dprevost-LMI dprevost-LMI marked this pull request as draft October 6, 2024 16:14
@dprevost-LMI dprevost-LMI force-pushed the develop-fix-leaking-promises branch from 9fb3eac to 74f98b2 Compare October 6, 2024 16:34
@dprevost-LMI
Copy link
Contributor Author

dprevost-LMI commented Oct 6, 2024

image

@dprevost-LMI dprevost-LMI marked this pull request as ready for review October 6, 2024 17:31
@dprevost-LMI dprevost-LMI changed the title Fix multiple leaking promises + bonuses Android : fix multiple leaking promises + bonuses Oct 6, 2024
@dprevost-LMI dprevost-LMI force-pushed the develop-fix-leaking-promises branch from 1682381 to 269b06d Compare October 6, 2024 17:36
@dprevost-LMI
Copy link
Contributor Author

@nilesh-simform Seeing that you cherry-pick changes from my PR into this #110. Can you give me some advice on the rest of the changes? They will never be chosen since they are too broad, so I close, or might they do a future cut?

chore: add ignore for vs cod plugin + comment annoying log

fix: continue better promise handling

chore: use err.toString() instead of err.message

fix: more better handling of promises

fix: forget to toString the duration

fix: remove deprecation + clean code of logd

fix: fix bad handling of onError and remove useless resolve/reject

chore: better detection of leaking promise with reject

fix: add wrong path case + fix rebase broken code

fix: unify error code in preparePlayer

chore: code simplification for player & player key usage and more

chore: final code review

fix: skip stop if the player is no more in the list

fix: forget to add the resolve back after using `getPlayerKeyOrReject`

chore: more code simplification + update ios pods with latest deps
@dprevost-LMI dprevost-LMI force-pushed the develop-fix-leaking-promises branch from 269b06d to d3dc28c Compare October 18, 2024 22:13
@dprevost-LMI dprevost-LMI changed the title Android : fix multiple leaking promises + bonuses Android : better promises handling + bonuses Oct 20, 2024
@dprevost-LMI dprevost-LMI marked this pull request as draft November 16, 2024 15:05
@dprevost-LMI dprevost-LMI deleted the develop-fix-leaking-promises branch November 16, 2024 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant