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

feat(web): add a web implementation #1886

Merged
merged 16 commits into from
Mar 7, 2024
Merged

Conversation

jspizziri
Copy link
Collaborator

@jspizziri jspizziri commented Jan 24, 2023

@dcvz here's the web implementation I've been working on. You can test it out via the following:

  1. Pull the branch, install deps, & build
  2. yarn example web:start

@github-actions
Copy link
Contributor

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale label Jul 27, 2023
@puckey puckey removed the Stale label Jul 27, 2023
@hirbod
Copy link

hirbod commented Aug 11, 2023

FYI: This has been unreviewed for about 8 months, but I decided to give it a try. I manually integrated all the web implementation parts into our app and made a few adjustments, such as preventing multiple Shaka instances being registered. I also added support for .pause (which was previously missing, super quick and dirty though) and ensured the emission of PlaybackProgressUpdated in that case.

Aside from that, the web component is quite solid. I'd love to see this integrated into the core (with some modifications) as it's very helpful and the API is almost identical.

In case someone is interested how I did that (it's ugly, since I had to copy some source files because of some resolving issues). showtime-xyz/showtime-frontend@staging...feat/track-player

I really like RNTP (beside of some hot reload issues) and its a solid library. I would love to see this merged, otherwise I'd be forced to maintain another fork, which is not helpful for the ecosystem imho.

Some recap:

  • pause was not working correctly
  • pause is not emitting PlaybackProgressUpdates
  • .add only accepts Track[] while the native part also accept Track
  • multiple setupPlayer calls are not prevented

Pretty sure I did not test all of the cases (we only play a single track at a time), but I have fixed those above

@dcvz
Copy link
Contributor

dcvz commented Aug 11, 2023

FYI: This has been unreviewed for about 8 months, but I decided to give it a try. I manually integrated all the web implementation parts into our app and made a few adjustments, such as preventing multiple Shaka instances being registered. I also added support for .pause (which was previously missing, super quick and dirty though) and ensured the emission of PlaybackProgressUpdated in that case.

Aside from that, the web component is quite solid. I'd love to see this integrated into the core (with some modifications) as it's very helpful and the API is almost identical.

In case someone is interested how I did that (it's ugly, since I had to copy some source files because of some resolving issues). showtime-xyz/[email protected]/track-player

I really like RNTP (beside of some hot reload issues) and its a solid library. I would love to see this merged, otherwise I'd be forced to maintain another fork, which is not helpful for the ecosystem imho.

Some recap:

  • pause was not working correctly
  • pause is not emitting PlaybackProgressUpdates
  • .add only accepts Track[] while the native part also accept Track
  • multiple setupPlayer calls are not prevented

Pretty sure I did not test all of the cases (we only play a single track at a time), but I have fixed those above

Thanks for checking it out @hirbod. I'm glad to hear it's working for you! This is definitely on our radar, and @jspizziri is part of the external core contributors and has been keeping us up to date on the work here. We're hard at work on the v4 release but I'm planning on taking a look at this once that's out :)

@jspizziri
Copy link
Collaborator Author

@hirbod please submit PR's for all the work that you've done to improve things to my fork and I'll review and merge them in there.

I've released my fork under @5stones/react-native-track-player and that's what I'm currently using (see the 4.0.0-web.x tags. I'll be maintaining my fork until it gets merged into the mainline repo, so I'd recommend working with me there so you don't have to do it solo). I periodically rebase it on the upstream and publish a new version in addition to releasing new versions when I have bug fixes etc. As @dcvz said, the hope is that we'll ultimately get this reviewed and merged once 4.0.0 is out of RC, at which time I'd abandon my fork and we'd all switch over to mainline.

I'd greatly appreciate PR's for the issues you've identified to feat/web-3.

@hirbod
Copy link

hirbod commented Aug 11, 2023

Thanks, @dcvz and @jspizziri, for the heads up. I'm glad to hear you guys have this on your radar.
@jspizziri, I regret not checking if your fork was actually published. Are you publishing from main? My codebase currently uses v3 for native, and, well, I suppose your implementation is from v4 :D

I can submit my minor bugfixes to your branch; they're straightforward.

@jspizziri
Copy link
Collaborator Author

@hirbod

Are you publishing from main?

It's... complicated 😄. But no, I'm maintaining feat/web-3 and publishing from publish. PR's are best sent to feat/web-3.

My codebase currently uses v3 for native, and, well, I suppose your implementation is from v4 :D

Yep, I'm using v4 (which I'd recommend for you as well). There aren't that many BC changes between 3 & 4 and we've published a number of RC's. It's pretty close to being a formal v4 release and many folks, use it in prod.

Regardless, any PR's you can send will make this a smoother process to get merged and released in the mainline in the coming months. Thanks!

@hirbod
Copy link

hirbod commented Sep 15, 2023

@jspizziri we started working on this feature again and I'd like to push my PRs to your repo. Would you mind to rebase to upstream and prepare a new version so I can incorporate some of the fixes?

@hirbod
Copy link

hirbod commented Sep 15, 2023

@jspizziri, after pulling in your branch, it appears that most of the issues I had are now resolved. I've PRs to your repo.

  1. Firstly, re-exporting was not possible (I've created a PR for this).
  2. setupPlayer isn't preventing multiple instances. I'll created another PR to address this.

@jspizziri
Copy link
Collaborator Author

@hirbod , I just did a rebase. It was a little messy due to all the upgrades done recently in the example app. It seems to be working however, please let me know if you find issues.

@dcvz just an FYI. Some of the improvements in the example app recently have caused some issues with web. Notably some things with the @gorhom/bottom-sheet BottomSheetScrollViews and some of the newer package version of react-native-reanimated (they're going through a pretty big transition with their typescript support). I might've glossed over some other things.

@dcvz
Copy link
Contributor

dcvz commented Sep 18, 2023

@jspizziri what kind of issues? Like those packages are not supported on web? or just typescript issues?

@jspizziri
Copy link
Collaborator Author

@dcvz we discussed this on our call, but to leave a paper trail.

The issues seemed to be specific with some of the new minor versions of the react-native-reanimated and react-native-gesture-handler packages. There are some internal structural issues and typescript issues that I believe are likely going to be resolved by them in the near future, so I pinned them to earlier minor versions so we don't have to worry about working around those issues for the time being. There's nothing fundamentally incompatible with them and web though, just existing bugs that will be resolved soon.

@sajorahasan

This comment was marked as outdated.

@jspizziri

This comment was marked as outdated.

@sajorahasan

This comment was marked as outdated.

@sajorahasan

This comment was marked as outdated.

@jspizziri

This comment was marked as outdated.

@jspizziri
Copy link
Collaborator Author

jspizziri commented Nov 28, 2023

@dcvz I just finished my audit of the web's API and here's what I found:

Currently Unimplemented

  • setQueue

Stubbed But Not Implemented

All the below stubbed methods can be seen here:

  • updateMetadataForTrack
  • updateNowPlayingMetadata
  • updateOptions

Deprecations

There are several deprecated methods that I didn't implement. I'm personally not in favor of implementing currently deprecated APIs.

Incorrectly Exposed

Currently, all of these getters/setters are public in the underlying Player class
but maybe shouldn't be, we could make them protected easily enough to lock them
down. However, they aren't exposed in the RNTP interface so getting at them would
be a chore if someone wanted to.

  • current
  • playWhenReady
  • state

Once you've taken a look at this LMK how you'd like to proceed and I can raise any issues in the issue tracker that we need to and fix whatever you want.

Copy link
Contributor

@dcvz dcvz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @jspizziri! Very clean implementation! I have mostly only minor comments. I did however notice that Whip (HLS) fails to load the duration / progress. More urgently though skipping past Whip throws an error. Is it perhaps the media type - next track is a live stream.

Expand Error Screenshot 2024-02-04 at 01 47 01

Also we probably (doesn't have to be part of this PR) want to leverage navigator.mediaSession to communicate with the OS and take advantage of media keys.

web/TrackPlayerModule.ts Outdated Show resolved Hide resolved
web/TrackPlayer/Player.ts Outdated Show resolved Hide resolved
@SpadarShut
Copy link
Contributor

In my project where I control the files played, I wouldn't want to pull all those extra dependencies when I just need to play mp3s. So I'd suggest to let developers configure support for extra formats as needed.

@jspizziri
Copy link
Collaborator Author

jspizziri commented Feb 7, 2024

In my project where I control the files played, I wouldn't want to pull all those extra dependencies when I just need to play mp3s. So I'd suggest to let developers configure support for extra formats as needed.

@SpadarShut , while allowing developers to configure support for extra formats sounds like an appealing option, the implementation of something like that would be quite tricky.

@SpadarShut
Copy link
Contributor

@SpadarShut , while allowing developers to configure support for extra formats sounds like an appealing option, the implementation of something like that would be quite tricky.

Sure, I get it. Just wanted to share my context, that it would be nice to avoid those extra deps if possible.

dcvz
dcvz previously approved these changes Mar 5, 2024
Copy link
Contributor

@dcvz dcvz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jspizziri this looks good to go on my end! Looks like CI typescript check is failing, will be happy to merge once that's green

@jspizziri
Copy link
Collaborator Author

@dcvz I fixed the ci in this commit and I rebased on main. Apart from that I didn't touch anything (there was a merge conflict as a result of this commit).

I think we should be good to go once the iOS job finishes taking its sweet time.

Copy link
Contributor

@dcvz dcvz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

time to merge this @jspizziri! 🎉

@dcvz
Copy link
Contributor

dcvz commented Mar 7, 2024

Awesome work here @jspizziri!

@dcvz dcvz merged commit 9aded62 into doublesymmetry:main Mar 7, 2024
5 checks passed
@jspizziri
Copy link
Collaborator Author

jspizziri commented Mar 7, 2024

@dcvz huzzah! What are your thoughts on a minor release?

Also, lmk if you'd like to see the docs site enhanced to incorporate the documentation (instead of just the readme).

@dcvz
Copy link
Contributor

dcvz commented Mar 11, 2024

@jspizziri i think I'll be making a minor release tomorrow which will include web -- if there are any things you'd like to add to the docs, let's do it. I can only think of a few places where we say we support iOS, Android and Windows.

@jspizziri
Copy link
Collaborator Author

@dcvz docs PR here

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.

6 participants