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.

Sign up for GitHub

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

Jump to bottom

doc: remove "is recommended" from crypto legacy API text #34697

Merged
merged 25 commits into from
Aug 18, 2020
Merged

doc: remove "is recommended" from crypto legacy API text #34697

merged 25 commits into from
Aug 18, 2020

Conversation

Trott
Copy link
Member

@Trott Trott commented Aug 9, 2020

The text for the legacy API sends mixed signals. It's legacy, but still
supported, so not deprecated, but not recommended. Let's begin to
clarify this by removing "not recommended". If we want to not-recommend
it, let's doc-deprecate it properly, or at least include an explanation
as to why it's not recommended.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. labels Aug 9, 2020
@Trott Trott changed the title doc: remove "is recommended from crypto legacy API text doc: remove "is recommended" from crypto legacy API text Aug 9, 2020
jasnell
jasnell requested changes Aug 10, 2020
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

If your recommendation is to doc deprecate it, then why not just do so in this PR? I'd prefer that over only removing the "not recommended" text

@Trott Trott force-pushed the crypto-recommended-1 branch from b264d46 to 9a6ed8c Compare August 16, 2020 16:57
@Trott
Copy link
Member Author

Trott commented Aug 16, 2020

OK, I've added a second commit that applies the documentation-only deprecation to the Legacy API. /ping @nodejs/crypto

(I'm leaving it as a separate commit in case there's push-back on the deprecation. Whether or not we deprecate, I think we should still clarify the wording so I want to make sure that stays even if we decide against deprecating.)

ryzokuken
ryzokuken approved these changes Aug 17, 2020
View reviewed changes
Copy link
Contributor

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

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

Thanks

mildsunrise and others added 4 commits August 17, 2020 13:49
@mildsunrise
http2: allow Host in HTTP/2 requests
f5c0e28 
The HTTP/2 spec allows Host to be used instead of :authority in
requests, and this is in fact *preferred* when converting from HTTP/1.

We erroneously treated Host as a connection header, thus disallowing
it in requests. The patch corrects this, aligning Node.js behaviour
with the HTTP/2 spec and with nghttp2:

 - Treat Host as a single-value header instead of a connection header.
 - Don't autofill :authority if Host is present.
 - The compatibility API (request.authority) falls back to using Host
   if :authority is not present.

This is semver-major because requests are no longer guaranteed to
have :authority set. An explanatory note was added to the docs.

Fixes: nodejs#29858

PR-URL: nodejs#34664
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
@clshortfuse @addaleax
http2: use and support non-empty DATA frame with END_STREAM flag
0e30c5b 
Adds support for reading from a stream where the final frame is a
non-empty DATA frame with the END_STREAM flag set, instead of hanging
waiting for another frame. When writing to a stream, uses a
END_STREAM flag on final DATA frame instead of adding an empty
DATA frame.

BREAKING: http2 client now expects servers to properly support
END_STREAM flag

Fixes: nodejs#31309
Fixes: nodejs#33891
Refs: https://nghttp2.org/documentation/types.html#c.nghttp2_on_data_chunk_recv_callback

PR-URL: nodejs#33875
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@lpinca @mmarchini
doc: fix broken links in commit-queue.md
92167e2 
Fixes: nodejs#34787

PR-URL: nodejs#34789
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Mary Marchini <oss@mmarchini.me>
@mmarchini
build: add CODEOWNERS linter action
8b818cf 
PR-URL: nodejs#34739
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
jasnell and others added 18 commits August 17, 2020 11:23
@jasnell
quic: resolve minor TODO in QuicSocket
1e6e5c3 
PR-URL: nodejs#34655
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@jasnell
quic: resolve some minor TODOs
34165f0 
PR-URL: nodejs#34655
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@jasnell
quic: limit push check to http/3
344c5e4 
PR-URL: nodejs#34655
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@jasnell
quic: fixup set_socket, fix skipped test
10d5047 
PR-URL: nodejs#34669
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell
quic: check setSocket natRebinding argument, extend test
442968c 
PR-URL: nodejs#34669
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell
quic: add natRebinding argument to docs
c17eaa3 
PR-URL: nodejs#34669
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell
quic: fixup session ticket app data todo comments
2405922 
PR-URL: nodejs#34741
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@jasnell
quic: resolve InitializeSecureContext TODO comment
19e712b 
Using a static label is sufficient.

PR-URL: nodejs#34741
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@jasnell
quic: clarify TODO statements
94aa291 
PR-URL: nodejs#34741
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@jasnell
quic: consolidate stats collecting in QuicSession
bfc3535 
PR-URL: nodejs#34741
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@jasnell
src: allow instances of net.BlockList to be created internally
1c14810 
Initial PR had it so that user code would create BlockList
instances. This sets it up so that instances can be created
internally by Node.js

PR-URL: nodejs#34741
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@jasnell
quic: use net.BlockList for limiting access to a QuicSocket
c855c3e 
PR-URL: nodejs#34741
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@mmarchini
meta: fix codeowners docs path
5835367 
s/docs/doc/g
Signed-off-by: Mary Marchini <mmarchini@netflix.com>

PR-URL: nodejs#34811
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@addaleax @jasnell
worker: do not crash when JSTransferable lists untransferable value
81df668 
This can currently be triggered when posting a closing FileHandle.

Refs: nodejs#34746 (comment)

PR-URL: nodejs#34766
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@cjihrig @jasnell
tools: update ESLint to 7.7.0
0eca660 
Update ESLint to 7.7.0

PR-URL: nodejs#34783
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott @jasnell
doc: edit filehandle.close() entry in fs.md
c62cf1d 
PR-URL: nodejs#34782
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Harshitha K P <harshitha014@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@lundibundi @jasnell
lib: allow to validate enums with validateOneOf
6726246 
PR-URL: nodejs#34070
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
doc,lib: remove unused error code
15fdd98 
As best as I can tell, ERR_V8BREAKITERATOR is unused anywhere in our
code base and dependencies. Move to legacy errors.

PR-URL: nodejs#34792
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@Trott
Copy link
Member Author

Trott commented Aug 18, 2020

@jasnell OK to clear your objection? I think the subsequent change aligns with your suggestion?

Trott added 3 commits August 17, 2020 22:09
@Trott
test: convert assertion that always fails to assert.fail()
aa5361c 
In test-http-destroyed-socket-write2, the assert.strictEqual() in the
default case of the switch statement will always fail because it checks
for a value that is already accounted for in one of the switch cases.
Convert it to assert.fail().

PR-URL: nodejs#34793
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
doc: remove "is recommended from crypto legacy API text
60d572c 
The text for the legacy API sends mixed signals. It's legacy, but still
supported, so not deprecated, but not recommended. Let's begin to
clarify this by removing "not recommended". If we want to not-recommend
it, let's doc-deprecate it properly, or at least include an explanation
as to why it's not recommended.

PR-URL: nodejs#34697
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@Trott
doc: deprecate (doc-only) crypto.Certificate()
ca5ff72 
Documentation-only: Recommend people use the static methods on
crypto.Certificate() and not the legacy API constructor.

PR-URL: nodejs#34697
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@Trott
Copy link
Member Author

Trott commented Aug 18, 2020

Landed in aa5361c...ca5ff72

@Trott Trott merged commit ca5ff72 into nodejs:master Aug 18, 2020
@Trott Trott deleted the crypto-recommended-1 branch August 18, 2020 05:12
BethGriggs pushed a commit that referenced this pull request Aug 20, 2020
@Trott @BethGriggs
doc: remove "is recommended from crypto legacy API text
a01011b 
The text for the legacy API sends mixed signals. It's legacy, but still
supported, so not deprecated, but not recommended. Let's begin to
clarify this by removing "not recommended". If we want to not-recommend
it, let's doc-deprecate it properly, or at least include an explanation
as to why it's not recommended.

PR-URL: #34697
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
BethGriggs pushed a commit that referenced this pull request Aug 20, 2020
@Trott @BethGriggs
doc: deprecate (doc-only) crypto.Certificate()
3b48d7a 
Documentation-only: Recommend people use the static methods on
crypto.Certificate() and not the legacy API constructor.

PR-URL: #34697
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@danielleadams danielleadams mentioned this pull request Aug 20, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Aug 20, 2020
@Trott @BethGriggs
doc: remove "is recommended from crypto legacy API text
3bfe199 
The text for the legacy API sends mixed signals. It's legacy, but still
supported, so not deprecated, but not recommended. Let's begin to
clarify this by removing "not recommended". If we want to not-recommend
it, let's doc-deprecate it properly, or at least include an explanation
as to why it's not recommended.

PR-URL: #34697
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
BethGriggs pushed a commit that referenced this pull request Aug 20, 2020
@Trott @BethGriggs
doc: deprecate (doc-only) crypto.Certificate()
47f2f45 
Documentation-only: Recommend people use the static methods on
crypto.Certificate() and not the legacy API constructor.

PR-URL: #34697
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
@Trott @addaleax
doc: remove "is recommended from crypto legacy API text
d0e7bf1 
The text for the legacy API sends mixed signals. It's legacy, but still
supported, so not deprecated, but not recommended. Let's begin to
clarify this by removing "not recommended". If we want to not-recommend
it, let's doc-deprecate it properly, or at least include an explanation
as to why it's not recommended.

PR-URL: #34697
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
@Trott @addaleax
doc: remove "is recommended from crypto legacy API text
7a983f5 
The text for the legacy API sends mixed signals. It's legacy, but still
supported, so not deprecated, but not recommended. Let's begin to
clarify this by removing "not recommended". If we want to not-recommend
it, let's doc-deprecate it properly, or at least include an explanation
as to why it's not recommended.

PR-URL: #34697
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@codebytere codebytere mentioned this pull request Sep 28, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryzokuken ryzokuken ryzokuken approved these changes

@tniessen tniessen tniessen approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.