Require statements in shim.js are not being evaluated conditionally
jd20 opened this issue · 9 comments
There are several require's in shim.js, that are supposed to only evaluate if react-native-crypto is in the package.json's dependencies. However, for me on RN 0.48.2, these require statements act like 'import', and are evaluated statically not dynamically, giving errors like "Unable to resolve module 'browserify-sign/algos' if react-native-crypto is not installed.
As a simple fix, based on some advice in an SO thread, I just placed the module names in variables, to force RN to evaluate the require's dynamically. I had some other unrelated changes on my fix branch, so having a hard time generating a clean PR, but the relevant commit is here: 8b8b715
@jd20 interesting, does that trick work with earlier versions of RN? I remember having some issue with dynamic requires on some version around 0.40-0.42...but don't remember what it was :)
another option would be to move the part inside the if to the react-native-crypto library, and do one dynamic require here, a la:
if (require('./package.json').dependencies['react-native-crypto']) {
const crypto = 'react-native-crypto'
require(crypto)
}maybe that would be cleaner? Then we wouldn't need dynamic requires all over the crypto piece
I think the dynamic require behavior changed around 0.40, but this trick shouldn't affect the behavior on older versions of RN.
So you're saying it's safe to move everything crypto-related in shim.js directly to react-native-crypto? I can take a stab at that.
i think so, but let me know how it turns out :)
Do you know the background on why browserify-sign is required? I seem to be able to do basic crypto stuff OK without requiring it, so having a hard time coming up with sample code to check if it's integrated correctly. Also, I needed to disable the crypto = window.crypto, this seems to cause problems because it sets global crypto to an empty shim, instead of react-native-crypto. Likewise, defining crypto.getRandomValues doesn't hurt anything, but seems unnecessary since node doesn't have it, so I'm wondering if there's a reason we want to keep that in shim.js?
Also, more minor thing, but seems we might want to add a note to the README to run react-native link after installing certain packages, since some like react-native-randombytes require it (took me a while to realize I needed to run it).
the browserify-sign bit was for one of my crypto projects. It doesn't actually require browserify-sign itself, it just adds an alias in the browserify-sign/algos section.
the rest of the crypto section is needed for modules that interpret their environment and decide that they're in the browser (take a look at brorand/index.js, which used to check explicitly for the window object). Once they decide they're in the browser, they want getRandomValues :) The problem with tricking them into thinking they're in Node.js is that then they assume they access to some synchronous apis - e.g. fs.readFileSync, etc.
about the react-native link bit, you're absolutely right, add it please!
I see, unfortunate that it's so messy :) I was confused at first why we'd want the node modules to think they were in the browser, but makes sense now. Will keep seeing what I can do with this.
Ok, submitted #54 for review. The react-native-crypto PR has to be merged first though, so please review that one first. I did some local testing (with both the updated react-native-crypto and rn-nodeify), using the sample randomBytes code in the README, and the getRandomValues() sample here: https://developer.mozilla.org/en-US/docs/Web/API/RandomSource/getRandomValues. If using window.crypto.getRandomValues(), you do need to import 'crypto' first, which seems reasonable to me.
resolved with #54 and react-native-crypto#19