Skip to content
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

fix(ts): add null to getCurrentTrack return type #1681

Merged
merged 2 commits into from
Sep 3, 2022

Conversation

qmx
Copy link
Contributor

@qmx qmx commented Aug 27, 2022

TrackPlayer returns null on iOS when current index is out of bounds/invalid

@qmx qmx requested a review from dcvz as a code owner August 27, 2022 18:39
@jspizziri
Copy link
Collaborator

@qmx this is a good catch. I just did some investigation on the Android side and it looks like the issue might be slightly larger.

Looking at how Android handles this it looks like it might return a -1 in these cases where iOS returns null. Would you be able to test and confirm that?

If that's the case then we might need to update that Kotlin function to check the same conditions as iOS so that the API's are consistent.

Thanks for the contribution!

@jspizziri jspizziri changed the title fix TS signature in TrackPlayer.getCurrentTrack() fix(ts): add null to getCurrentTrack return type Aug 29, 2022
@qmx
Copy link
Contributor Author

qmx commented Aug 31, 2022

Looking at how Android handles this it looks like it might return a -1 in these cases where iOS returns null. Would you be able to test and confirm that?

Testing android is harder for me - I don't even know how could I repro that tbh.

Would it make sense to actually get iOS to return the -1 too and deal with this at the react native level?

@dcvz
Copy link
Contributor

dcvz commented Sep 2, 2022

Definitely think parity would be good. We can start with having both platforms return -1 if there’s no current track.

@qmx qmx force-pushed the fix-getCurrentTrack-type branch from 8358331 to 5ff2f81 Compare September 2, 2022 17:51
@qmx qmx requested a review from mpivchev as a code owner September 2, 2022 17:51
@qmx
Copy link
Contributor Author

qmx commented Sep 2, 2022

@dcvz I ended up updating the Android API behavior to match iOS

@qmx qmx force-pushed the fix-getCurrentTrack-type branch from 5ff2f81 to a477c85 Compare September 2, 2022 18:37
@qmx qmx requested review from dcvz and removed request for mpivchev September 2, 2022 18:42
dcvz
dcvz previously approved these changes Sep 2, 2022
@qmx
Copy link
Contributor Author

qmx commented Sep 3, 2022

fixed the linter/prettier formatting error

@qmx qmx requested a review from dcvz September 3, 2022 18:38
@dcvz
Copy link
Contributor

dcvz commented Sep 3, 2022

Thanks for this @qmx !

@dcvz dcvz merged commit 096ec68 into doublesymmetry:main Sep 3, 2022
praisedavid787 added a commit to praisedavid787/react-native-track-player that referenced this pull request Sep 5, 2022
fix(ts): add `null` to getCurrentTrack return type (doublesymmetry#1681)
praisedavid787 added a commit to praisedavid787/react-native-track-player that referenced this pull request Sep 5, 2022
praisedavid787 added a commit to praisedavid787/react-native-track-player that referenced this pull request Sep 5, 2022
Revert "fix(ts): add `null` to getCurrentTrack return type (doublesymmetry#1681)"
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.

3 participants