Use isomorphic-webcrypto (enable React Native). #43
Conversation
Codecov Report
@@ Coverage Diff @@
## master #43 +/- ##
=======================================
Coverage 90.22% 90.22%
=======================================
Files 10 10
Lines 624 624
=======================================
Hits 563 563
Misses 61 61 Continue to review full report at Codecov.
|
dmitrizagidulin
changed the title
lib/MessageDigest-browser.js
Outdated
| const crypto = self.crypto || self.msCrypto; | ||
|
|
||
| // TODO: synchronous version no longer supported in browser | ||
| const crypto = require('isomorphic-webcrypto'); |
There was a problem hiding this comment.
How much does this bloat the package size? Is there a way for us to only load this for react-native? We had considerable size savings by just using native implementations before. Shouldn't react-native apps just pull this in on their own and expose it as a global?
There was a problem hiding this comment.
Always a good question (re package size). Far as I can tell: https://bundlephobia.com/result?p=isomorphic-webcrypto@2.3.8 (10.5k minified, 3.7k gzip'd).
Pulling in crypto as a global, as a technique, is not a bad idea, worth exploring.
(Also, I very much wish that React Native had a similar section that the "browser" has in package.json. As far as I can tell, it just uses the "browser" section, actually. Need to investigate.)
There was a problem hiding this comment.
Ahhh you know what, RN straight up has a "react native" section of package.json, that acts exactly like the browser section.
There was a problem hiding this comment.
@dlongley should be fixed now.
| @@ -3,11 +3,7 @@ | |||
| */ | |||
| 'use strict'; | |||
|
|
|||
| require('setimmediate'); | |||
There was a problem hiding this comment.
It's weird that this was put here, as I think it may have been required for some other reason (i.e., this function isn't supported in browsers but we use it elsewhere). @davidlehn, can you speak to this?
There was a problem hiding this comment.
setImmediate is used in the code, so a polyfill is needed. See lib/URDNA2015.js. This polyfill replaced custom code and was added in:
4e10528
The browser testing of this package is indirect through jsonld.js, to avoid complexity of duplicating the testing framework. A side effect of that seems to be confusion as to why this is needed. jsonld.js tests will fail without this.
I'm not sure why it's in this file in particular. I probably thought it easier to just use this file rather than create a new empty node file and a browser only file with just that require.
Please restore. If a comment or moving to a new browser only file would help avoid confusion, that's fine.
There was a problem hiding this comment.
Ahhh got it, thanks!!
There was a problem hiding this comment.
@davidlehn interestingly, requiring setimmediate causes to fail a build in my case. I could provide a repro if you are interested and would be interested in understanding why.
There was a problem hiding this comment.
Ok, thanks, this looks like it shouldn't affect the non-react-native builds now.
I still think it's strange that we have to modify our package to enable crypto in react-native ... shouldn't it be more of a polyfill scenario where one's react-native app just includes isomorphic-webcrypto and exposes crypto globally? I'd think you'd just want to do that once, instead of having every package that uses browser crypto have to cater to react-native. If that's the only way forward I can live with it for now -- and hopefully it will be addressed in the future and we can remove the cruft here.
We should hold this PR until @davidlehn can comment on the removal of setimmediate, however. I suspect that breaks browser builds? I don't think any browser still implements setimmediate since Edge switched to use Chromium as a base. setimmediate was previously only implemented in MS browsers -- and in node.js.
Ideally karma tests would catch this ... but maybe webpack is already magically polyfilling.
| @@ -26,7 +26,8 @@ | |||
| "lib/*.js" | |||
| ], | |||
| "dependencies": { | |||
| "setimmediate": "^1.0.5" | |||
There was a problem hiding this comment.
@davidlehn -- can you speak to this?
There was a problem hiding this comment.
Restore the dependency, it's needed. See the MessageDigest-browser.js comments.
| "./lib/MessageDigest-reactnative.js": false, | ||
| "fast-text-encoding": false, | ||
| "isomorphic-webcrypto": false, | ||
| "rdf-canonize-native": false | ||
| }, | ||
| "react-native": { | ||
| "./lib/MessageDigest.js": "./lib/MessageDigest-reactnative.js", | ||
| "./lib/MessageDigest-browser.js": false, |
There was a problem hiding this comment.
I think this can be simpler. No need to mask out deps that will never be required. The rdf-canonize-native one is needed since that is speculatively loaded. (And that code pattern could be avoided too.)
| "./lib/MessageDigest-reactnative.js": false, | |
| "fast-text-encoding": false, | |
| "isomorphic-webcrypto": false, | |
| "rdf-canonize-native": false | |
| }, | |
| "react-native": { | |
| "./lib/MessageDigest.js": "./lib/MessageDigest-reactnative.js", | |
| "./lib/MessageDigest-browser.js": false, | |
| "rdf-canonize-native": false | |
| }, | |
| "react-native": { | |
| "./lib/MessageDigest.js": "./lib/MessageDigest-reactnative.js", |
|
Can this be closed? It seems like the wrong way to solve the problem. Shouldn't react-native apps just polyfill the WebCrypto API and then all other libs that depends on it will work without having to modify each of those libs independently? |
|
Closing based on this comment: #43 (comment) |
|
@dlongley currently it's not possible to polyfill Edit: there is a polyfil for React Native to replace node's crypto, but not WebCrypto. It's usable. |
|
@pleerock fwiw, we use https://www.npmjs.com/package/isomorphic-webcrypto as a WebCrypto polyfill in react native. |
Fixes issue #42.