Skip to content
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

Fix for issue #602. Chrome's window.crypto property is read-only, so … #782

Closed
wants to merge 2 commits into from

Conversation

ben-yocum
Copy link

…when Electron tries to require Node's crypto library, it fails. The fix is identical to that for node-webkit.

…ead-only, so when Electron tries to require Node's crypto library, it fails.
@mattcollier
Copy link
Contributor

Thank you for the PR. I think this logic probably belongs in the isNodejs utility here:

https://github.com/digitalbazaar/forge/blob/master/lib/util.js#L110-L111

@ben-yocum
Copy link
Author

@mattcollier Sounds fine to me. PR updated.

@mattcollier
Copy link
Contributor

@gitsage it's my understanding that Electron provides two contexts where you main run code. The main process and a renderer process. The main process can load the node crypto module according to this comment.

This article claims the full node API is available in the main process.

I suspect the difficulty here is in the renderer process which does not include the full Node API?

Does forge function properly in one execution context and not in the other? If so, does this patch properly discern between the two?

@ben-yocum
Copy link
Author

@mattcollier I have investigated and you are correct, the main process can load the node crypto module correctly. The renderer process cannot.

The process of moving node-forge into the main thread was a pretty big pain. You have to perform asynchronous communication with the main thread through ICP, which carries a performance hit with it.

I think this PR should be closed without merging. Those who need this behavior should use the existing usePureJavaScript flag.

@mattcollier
Copy link
Contributor

@gitsage thanks for the follow-up. Closing.

@mattcollier mattcollier closed this Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants