Skip to content
This repository has been archived by the owner on Jul 21, 2023. It is now read-only.

Improve speed #117

Merged
merged 5 commits into from
Jan 27, 2018
Merged

Improve speed #117

merged 5 commits into from
Jan 27, 2018

Conversation

richardschneider
Copy link
Contributor

jsrsasign is 2 orders of magnitude slower that node-forge; see the benchmarks.

node-forge is a little bigger than jsrsasign (273 kB vs 259 kB).

libp2p-crypto relies upon keypair for RSA. It is 128 kB. keypair has copied node-forge code from 5 years ago and has added JWK support. If we make a JWK PR to forge, then we can remove the the keypair dependency.

@ghost ghost added the status/in-progress In progress label Dec 25, 2017
@daviddias
Copy link
Member

@richardschneider have you tried npm linking this package to its dependents and run the tests? The tricky part with UMD comes appears when you try to require a module that has a dependency using it.

@richardschneider
Copy link
Contributor Author

forge is now CommonJS so UMD issues disappear. All tests pass on all platforms and CIs.

Not sure what you want to npm link? I have npm link crypto to keychain and it works.

@daviddias
Copy link
Member

daviddias commented Dec 27, 2017

Interesting, that seems to be the case now https://www.npmjs.com/package/node-forge#installation

I'll test this ASAP. Thanks @richardschneider :)

@richardschneider
Copy link
Contributor Author

@diasdavid Thanks. IMHO node-forge is a better package than jsrsasign

@richardschneider
Copy link
Contributor Author

@diasdavid LGTM

@richardschneider
Copy link
Contributor Author

@diasdavid This is open for over a month. Are there any issues?

Copy link
Member

@daviddias daviddias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small note. Testing it now

@@ -121,21 +121,30 @@ class RsaPrivateKey {
* @returns {undefined}
*/
export (format, password, callback) {
const self = this
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never gets used.

@ghost ghost assigned daviddias Jan 27, 2018
@daviddias daviddias merged commit cdcca5f into master Jan 27, 2018
@daviddias daviddias deleted the forge branch January 27, 2018 18:54
@ghost ghost removed the status/in-progress In progress label Jan 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants