-
-
Notifications
You must be signed in to change notification settings - Fork 224
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
Conversation
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. |
hey @monokaijs! thanks for the feedback – appreciate it! |
How do you think about customizable cache adapter? Or allow caching customizations? |
@monokaijs You can already pass your own implementation of the ICache interface to YouTube.js' |
} | ||
|
||
#getStorage() { | ||
const storage = new ((globalThis as any).mmkvStorage as any)({ id: 'InnertubeCache' }); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree
There was a problem hiding this comment.
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
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. |
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 |
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. |
I think Browser Implementation is enough. |
Hey @LuanRT I think it might work with browser implementation directly by passing custom 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 tl;dr I think having this merged would clearly benefit @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 |
Thanks! : ) |
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.