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

doc: improvements to crypto.markdown copy #5230

Closed
wants to merge 1 commit into from
Closed

doc: improvements to crypto.markdown copy #5230

wants to merge 1 commit into from

Conversation

estliberitas
Copy link
Contributor

Fix several typos. Add missing links.

@mscdex mscdex added crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. labels Feb 15, 2016
@estliberitas
Copy link
Contributor Author

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.

@jasnell
Copy link
Member

jasnell commented Feb 15, 2016

LGTM

@Knighton910
Copy link

LGTM

1 similar comment
@jasnell
Copy link
Member

jasnell commented Feb 19, 2016

LGTM

@estliberitas
Copy link
Contributor Author

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

@Knighton910
Copy link

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.

@jasnell
Copy link
Member

jasnell commented Feb 19, 2016

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.

@jasnell
Copy link
Member

jasnell commented Feb 19, 2016

@nodejs/crypto

@bnoordhuis
Copy link
Member

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

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

@estliberitas
Copy link
Contributor Author

@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?

@bnoordhuis
Copy link
Member

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.

@bnoordhuis
Copy link
Member

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.

@estliberitas
Copy link
Contributor Author

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

@bnoordhuis
Copy link
Member

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.

@jasnell
Copy link
Member

jasnell commented Feb 20, 2016

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.

@ChALkeR
Copy link
Member

ChALkeR commented Feb 21, 2016

@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.

@estliberitas
Copy link
Contributor Author

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

@jasnell
Copy link
Member

jasnell commented Feb 21, 2016

@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!

@estliberitas
Copy link
Contributor Author

@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?

@johanneswilm
Copy link

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

@bnoordhuis
Copy link
Member

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.

@estliberitas
Copy link
Contributor Author

@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?

@bnoordhuis
Copy link
Member

Correct.

@ChALkeR
Copy link
Member

ChALkeR commented Feb 21, 2016

@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.

@estliberitas
Copy link
Contributor Author

@bnoordhuis

Correct.

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

@bnoordhuis
Copy link
Member

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

@@ -716,7 +717,7 @@ console.log(sign.sign(private_key, 'hex'));
### sign.sign(private_key[, output_format])

Calculates the signature on all the data passed through using either
`sign.update()` or `sign.write()`.
[`sign.update()`][] or [`sign.write()`][writable-write].
Copy link
Contributor

Choose a reason for hiding this comment

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

where does this writable-write link to?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, found it. maybe name it stream-writable-write for clarity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@silverwind
Copy link
Contributor

LGTM except suggestion above.

@silverwind
Copy link
Contributor

@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

Fix several typos. Add missing links.
@estliberitas
Copy link
Contributor Author

@silverwind Done.

silverwind pushed a commit that referenced this pull request Feb 28, 2016
Fix several typos. Add missing links.

PR-URL: #5230
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Kelvin Knighton <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
@silverwind
Copy link
Contributor

Thanks! Landed in f0c0614.

@silverwind silverwind closed this Feb 28, 2016
@estliberitas estliberitas deleted the doc-fix-crypto branch February 28, 2016 21:13
@Fishrock123 Fishrock123 mentioned this pull request Mar 1, 2016
5 tasks
MylesBorins pushed a commit that referenced this pull request Mar 1, 2016
Fix several typos. Add missing links.

PR-URL: #5230
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Kelvin Knighton <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 1, 2016
Fix several typos. Add missing links.

PR-URL: #5230
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Kelvin Knighton <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Mar 2, 2016
Fix several typos. Add missing links.

PR-URL: #5230
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Kelvin Knighton <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
Fix several typos. Add missing links.

PR-URL: #5230
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Kelvin Knighton <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants