bzarras / newsapi

A promise-based node interface for NewsAPI
MIT License
146 stars 39 forks source link

React Native #7

Closed agersoncgps closed 6 years ago

agersoncgps commented 6 years ago

Is this module compatible with React Native? When I try to use it I get several errors about modules such as querystring and crypto missing, which appear to be dependencies of this module and are normally installed automatically by npm.

error: bundling failed: Error: Unable to resolve module querystring from /node_modules/newsapi/index.js: Module does not exist in the module map

If I manually install them I eventually get stuck as well.

bzarras commented 6 years ago

@agersoncgps Compatibility with React Native was not something I originally considered when creating this module, but I can look into what it would take to make it compatible

cmmartin commented 6 years ago

The crypto module seems to be the only issue, since it's not available in react native. I don't see it being used anywhere in this project though. Any idea what is relying on crypto?

cmmartin commented 6 years ago

I figured it out. The request module is importing crypto. Switching to node-fetch should make it work in react native

bzarras commented 6 years ago

Yeah I figured it was probably request. I'll make the change and let you know when it's up

bzarras commented 6 years ago

@cmmartin Just pushed a patch release that uses node-fetch instead of request. Let me know if you are still experiencing problems, then I'll close out this issue

cmmartin commented 6 years ago

I tested this out in react-native and it throws this error Unhandled rejection TypeError: fetch(...).then(...).spread is not a function

I know this is odd, but I believe it is because React native doesn't run node. It executes javascript with JavaScriptCore, which has no networking layer.

So, RN provides fetch itself and overrides any reference to the globalfetch during the packaging phase. This means in RN, you aren't actually using node-fetch but their custom implementation which doesn't let you override .Promise.

Hopefully that makes sense. Regardless, I sent a pull request that I have confirmed works in both React Native and node 6+. All the tests are passing as well.

cmmartin commented 6 years ago

Essentially the PR just replaces .spread with...

.then(res => Promise.all([res, res.body]))
.then(([res, body]) => { ... }
bzarras commented 6 years ago

@cmmartin nice work! Your pull request is merged. Let me know if everything is working for you now and then I will close out this issue.

cmmartin commented 6 years ago

It works 🙌🏻 Thanks for pushing this through

bzarras commented 6 years ago

Great! No problem!