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

Add support for react-native platform #593

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nugmanoff
Copy link

Following PR adds support for react-native as a target platform. Thank you for making the library platform-agnostic and including the guide on how to add support for a new one – it helped a ton!

I've added some docs explaining what steps need to be taken on RN side in order for things to work.

There are some rough edges around typing that could easily be fixed. Decided to throw it up here first to get some feedback.

@monokaijs
Copy link

Your work is awesome, however I think you should make mmkv optional. Forcing using mmkv make this module unable to be used along with react-native-debugger (since mmkv does not work without jsi). Request response from Innertube always complicate to debug, missing the help from network debugging and a good console viewer like react-native-debugger could be a big deal with us.

@nugmanoff
Copy link
Author

hey @monokaijs! thanks for the feedback – appreciate it!
I am pretty newbie-ish when it comes to RN, so good to know the limitations of mmkv that you've mentioned; what alternatives would you suggest to use as a backing storage for cache to be used with this Youtube.js?

@monokaijs
Copy link

monokaijs commented Feb 25, 2024

How do you think about customizable cache adapter? Or allow caching customizations?
I used to try to use youtubei.js in my RN app, at that time, I used AsyncStorage as cache engine and it worked flawlessly. I think you should make Caching optional since we can implement our own cache mechanism, in this case, I see mmkv is just an option of developer, not the library itself.

@absidue
Copy link
Collaborator

absidue commented Mar 31, 2024

@monokaijs You can already pass your own implementation of the ICache interface to YouTube.js' InnerTube.create method, this pull request just provides a default implementation, through the UniversalCache class. YouTube.js won't use any cache unless you pass an instance of a cache, so it won't cause any problems.
I think this is fine as it is.

}

#getStorage() {
const storage = new ((globalThis as any).mmkvStorage as any)({ id: 'InnertubeCache' });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be worth throwing an error with an explanation when mmkvStorage is missing, that will prevent any cryptic errors further down the line.

Choose a reason for hiding this comment

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

Agree

Copy link
Author

Choose a reason for hiding this comment

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

@absidue makes sense! I've just left a #593 (comment) and if this is the only issue that is blocking as per @LuanRT consideration – I will get it fixed asap

@monokaijs
Copy link

I see no need this implementation anymore. With React Native, after adding some polyfills and configure metro and builder to support YouTube.js, you can use this module without any hassle.
I've successfully implemented this module into my project with just some polyfills (which are also available in this PR). I think no big difference between ReactNative and browser platform except fetch function (which we do not have to proxy in ReactNative since security headers are modificable).

Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. Remove the stale label or comment or this will be closed in 2 days

@github-actions github-actions bot added the Stale label May 31, 2024
@LuanRT
Copy link
Owner

LuanRT commented Jun 1, 2024

Sorry for letting this get stale, I need some opinions on whether this is still needed or not. The last comment in this thread makes me think the browser implementation is enough, but I have no experience with react native to verify that.

I already reviewed this a while ago so I'll merge it once we come to a conclusion.

@monokaijs
Copy link

I think Browser Implementation is enough.

@nugmanoff
Copy link
Author

nugmanoff commented Jun 5, 2024

Hey @LuanRT I think it might work with browser implementation directly by passing custom Cache implementation in Innertube.create call. But I believe it is more about the bigger picture of indicating that YouTube.js is supported and can be run on react-native and having an out-of-the box way to do that (i.e. providing default Cache implementation using react native specific libraries and necessary documentation).

I think the last documentation bit on glueing everything together on the React Native runtime side (using polyfills) is the most important thing to consider here. It took me quite some time to figure out how to properly pick and apply polyfill libraries on React Native side and it also includes some configs around bundler and babel. But now basically anyone who is looking for using YouTube.js can just install it as a dependency (and leverage the default Cache impl) and drop the polyfills setup file that is presented in the docs and have everything up and running in a few minutes. Without it one would need to figure out the Cache and polyfills for themselves (and might very well give up on the last one, thinking that it is not compatible with RN runtime at all).

tl;dr I think having this merged would clearly benefit YouTube.js by making the fact that it could be used and run on React Native more explicit and whole set up process more streamlined and drop-in, at the expense of having almost similar to web platform setup file (except for Cache implementation) – so almost no maintenance overhead.

@monokaijs thank you for your thoughts on this one as well! while I do agree with you to some extent, I think the fact that you have discovered my fork because it was opened as PR in this repo and that helped you have YouTube.js running in your RN project only highlights the fact that why it is important to have this explicit support merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants