-
Notifications
You must be signed in to change notification settings - Fork 13
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
Enable support for using this in React Native #42
Comments
Thanks for the callout @dmitrizagidulin! I have come across this issue as well. Any workarounds in the meantime? |
I'll note that DigitialCredential's fork released under |
Thanks for your response @JamesKEbert! Yes, I was able to come to the same conclusion a couple weeks ago. Regarding your issue with
Hope this helps! |
We don't use currently use react native at Digital Bazaar, so figuring out these support issues and testing is a bit difficult. The mass forking seems a bit unfortunate. I hope there's a better solution here. We've been in the process of converting all our other packages to ESM, and are trying to make everything run in modern browsers and recent Node.js versions. We'll get to jsonld, jsigs, and this package soon. And I hope that addresses some of the problems. It would be great if someone could help with figuring out what a minimal react native support patch would be. And perhaps also let us know how to test it. |
@davidlehn completely understand your points on supporting and testing react-native specific issues! But the fix is relatively simple and it was tested by at least our team and @kezike as well who proposed the root of the fix. Using The actual fix is just a single line of code change in |
@pleerock It's never "just a single line". :-) Please do propose a PR if the changes are not invasive! If special RN support is needed, we also need a "npm run test-rn", or similar script, that tests RN support is working. As some time has passed, what are the current issues using this package as-is? We really would prefer a straightforward approach of writing code that uses standard web APIs as much as possible with minimal platform specific support and dependencies. Applications should polyfill standard APIs as needed on their platforms. In this package we do have Node.js crypto and Web Crypto API support. We would only use only the Web Crypto API, but it turns out the Node.js crypto API performs better for our lots-of-hashing-calls use case. I see your comment saying that a Web Crypto API polyfill is not possible in React Native? What is stopping them from fixing that? I had been hoping they would update their system such that RN support here would be only adding a RN override to the webcrypto code paths. In that case, it might only be a single line fix, excluding testing cruft. In general, we don't have the resources to spend much time on this issue. I still don't know how to even test this in RN. About my "ESM soon" comments... ahem... maybe not so soon! It's on our roadmap though. |
One small correction: there is actually a polyfill for React Native to replace Obviously There are polyfills which we can use, and usually rn-nodeify is used to resolve issues which can be resolved by exist polyfills. The way Particularly in this case it adds the following lines to "react-native": {
"./lib/MessageDigest.js": "./lib/MessageDigest-browser.js",
// ...
} It happens because "browser": {
"./lib/MessageDigest.js": "./lib/MessageDigest-browser.js",
"rdf-canonize-native": false
} But it cannot really understand that in this particular case browser-based code won't work in react-native because Solution is simple as if we add a directive to original rdf-canonize's package.json to use "react-native": {
"./lib/MessageDigest.js": "./lib/MessageDigest.js"
} |
Of course that's what we always want to have. But reality is rough. Especially with cryptography.
React Native is a big and very very slow project. I guess it will take another 10 years for them to ship spec-complaint crypto-implementation out of the box.
Suggested solution is exactly what you've asked. Single line fix in paths.
Completely understand. It's another big environment to test and maintain... But since project is open-sourced and community contribute, sometimes we have to trust the community to add the changes and trust them to cover some of the manual testing we have to do in environments, such as react native. To make the project better. You just need to be sure it's not something over-complicated or breaking some of the other stuff. And the changes I propose exactly meet that criteria.
I would put ESM as a top-level priority in your case. Non-ESM libraries these days bring too many problems to their users. |
@davidlehn I can create a PR if you are okay with the proposed changes in |
Update - @pleerock proposedchange to |
Currently fails on React Native (since that environment has neither node's crypto nor the browser's
crypto.subtle.
.Instead, convert to use
isomorphic-webcrypto
lib, which also has React Native support - https://github.com/kevlened/isomorphic-webcrypto#react-native.The text was updated successfully, but these errors were encountered: