Skip to content
This repository has been archived by the owner on Mar 11, 2022. It is now read-only.

Hard dependency on react-native #83

Open
benkaiser opened this issue Jun 23, 2021 · 5 comments
Open

Hard dependency on react-native #83

benkaiser opened this issue Jun 23, 2021 · 5 comments

Comments

@benkaiser
Copy link
Contributor

Hey @AbelTesfaye,

I see in the last update to this repo, a hard dependency was taken on the 'react-native' package in url/index.js.

This has broken the build where I use react-native-ytdl in several web applications with no hard-dependency on react-native. Is there a possible fix we could consider that does not take a hard dependency on the react-native package itself?

Thanks!

@benkaiser
Copy link
Contributor Author

In case anyone else runs into this issue and needs a browser-only version, here is my fork: https://github.com/benkaiser/react-native-ytdl

@benkaiser
Copy link
Contributor Author

@AbelTesfaye I found an issue in how the cache was storing promises for the identity tokens. Looks like we need to make the getOrSet function in cache.js an async function, move that await on the value to the main part of the function, and then only set the value after the awaiting is done (so it isn't a promise anymore).

See my changes: benkaiser@f860f6d

I also made json page first, since it seemed to more reliably return results instead of the HTML option. Should we instead switch it to a Promise.race, and wait for the first one that succeeds to win? Rather than waiting a bunch of retires on one method before failing to the next?

@AbelTesfaye
Copy link
Collaborator

Hey @benkaiser,

I don't think we can avoid the react-native dependency since the URL polyfill will not work on IOS 10 without it plus there are some other dependencies that need it i.e. BlobModule

@AbelTesfaye
Copy link
Collaborator

This library doesn't use getOrSet synchronously anywhere, but if it will sometime in the future, then it's going to fail. Below is what I mean by synchronous usage:

const c = new Cache()
const cReturn = c.getOrSet("MY_KEY", () => "test")
console.log('cReturn:', cReturn) // it should return "test" but will instead return a promise object

The bug will be subtle and will be difficult to find later on, might be best not to modify the method in the first place.

@AbelTesfaye
Copy link
Collaborator

I think a few versions back, the JSON page was failing for certain videos, hence why the HTML page has more precedence. This might have changed now but i'm not sure. But for react-native-ytdl, I think using the same strategy as node-ytdl-core will be better in the long run since it will require minimal changes to be made when new versions need to be ported.

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

No branches or pull requests

2 participants