Also, guys. While I've been working on this I've got a question about crypto.Certificate. Why is it not a static class or just an object with functions?
I could not find any reason for using crypto.Certificate as a class, because:

1. crypto.Certificate just wraps C++ Certificate class
2. C++ Certificate methods do not work with instance, just extract environment from it (one method only - Certificate::VerifySpkac())

So I guess there should be a reason for this. Like inheriting of AsyncWrap or MakeWeak makes sense with memory or handle management or so.

LGTM

LGTM

LGTM

Guys, can anyone answer question above? Or should I create separate issue for this q?

Personally, I'd open a separate Issue for it. That way someone will get to it faster & I'd cc some of the guys who worked on the crypto doc. But someone should get to it here, as well. This issue does have a crypto label on it.

Not sure of the history on that class, it's likely just some left over legacy thing. Perhaps @indutny or @shigeki could shed some light on it.

@nodejs/crypto

Why is it not a static class or just an object with functions?

No reason except that it follows how other types are implemented.

@bnoordhuis Would it be a good idea to simplify things and, for example, put Certificate instance methods in crypto module? Or this is a case when beauty of consistency is preferred?

If it was new code, I would have pushed to implement it as static methods. But it's existing, working code; it would basically be an aesthetic change.

aesthetic change

Then again, maybe not. Implementing it as static methods would eliminate the need for a persistent handle and that's never a bad thing.

@bnoordhuis if it would be a change for the good of everyone, I'd like to implement it and let others judge it.

Go ahead, I'll review it. Please make sure you read CONTRIBUTING.md first and that make test passes before you open the pull request. Cheers.

We should first attempt to see how extensively the current API is used (cc @ChALkeR). The current API form would need to be deprecated. Seems like a good change to make.

@estliberitas Can you split the actual (non-doc) change proposal into a separate issue, please?

@jasnell This is everything that I found for crypto.*\.Certificate (downloads/month on the left):

14264   diff-dom-2.0.0.tgz/venv/src/node-v4.1.1/test/parallel/test-crypto-certificate.js:21:var certificate = new crypto.Certificate();
483     webid-0.3.7.tgz/tls/index.js:12:var certificate = new crypto.Certificate()
483     webid-0.3.7.tgz/test/test.js:8:var certificate = new crypto.Certificate()
434     pn-1.0.0.tgz/crypto.js:5:  Certificate: { enumerable: true, value: crypto.Certificate },
60      biojs-vis-blast-0.1.5.tgz/node/test/simple/test-crypto-certificate.js:37:var certificate = new crypto.Certificate();
43      flush-all-0.1.1.tgz/node-v0.13/test/simple/test-crypto-certificate.js:37:var certificate = new crypto.Certificate();
38      node-core-test-simple-0.11.11.tgz/test-crypto-certificate.js:37:var certificate = new crypto.Certificate();

The first line actually looks as a diff-dom packaging error to me, I'm going to report that now.

Even given the false negatives, the usage looks very low, so I think that the change is possible here. It would still require several major releases, of course.

Cool, okay, I'll be working on it today.

@ChALkeR +1 thank you for that. So yeah @estliberitas, The existing API would need to go through a deprecation cycle (do a quick search for "deprecated" in the source to see how that's handled in code). The documentation would also need to be updated to show that. Deprecations are a semver-major change so it would hit as part of v6. You can introduce the revamped API alongside the deprecation but the existing stuff cannot be removed until at least v7 or v8. If you need any help, let one of us know!

@jasnell Okay, I want to walk the whole way to the new API even if it will be released with v8 or so. I'll check soon how deprecations were handled previously. I guess, it is to be a series of 2 PRs:

1. Wrap existing API in internal/util.deprecate() and introduce another API.
2. Remove old API.

Correct?

The first line actually looks as a diff-dom packaging error to me, I'm going to report that now.

Indeed. Should be fixed in version 2.0.1

Maybe I didn't make myself clear but I wouldn't change the public API, just the implementation details so the persistent handle is no longer necessary.

@bnoordhuis

Then again, maybe not. Implementing it as static methods would eliminate the need for a persistent handle and that's never a bad thing.

So this is about binding implementation?

Correct.

@jasnell

Deprecations are a semver-major change so it would hit as part of v6. You can introduce the revamped API alongside the deprecation but the existing stuff cannot be removed until at least v7 or v8.

I'd go with a documentation-only deprecation in v6, actual deprecation in v7 (perhaps), and possible removal at some point after that (in v8 or later).

These npm greps are not the absolute source of information — someone could be using this in their private code (e.g. their website apps etc). Also, there could be false negatives even in the data that I have.

We should take into account that crypto module is labeled Stable in the docs.

@bnoordhuis

Correct.

Would it be semver-(minor|major)? I guess, no, thus no new public API is added and existing is not changed.

It could be semver-patch but because it's not a real bug fix, I'd be conservative and label it semver-minor.

LGTM except suggestion above.

@estliberitas still not applying cleanly here:

Error: git am exited with code: 128
.git/rebase-apply/patch:590: trailing whitespace.
[`ecdh.setPrivateKey()`]: #crypto_ecdh_setprivatekey_private_key_encoding
.git/rebase-apply/patch:596: trailing whitespace.
[`hmac.digest()`]: #crypto_hmac_digest_encoding
error: patch failed: doc/api/crypto.markdown:899
error: doc/api/crypto.markdown: patch does not apply

@silverwind Done.

Thanks! Landed in f0c0614.