doc: remove "is recommended" from crypto legacy API text by Trott · Pull Request #34697 · nodejs/node

Trott · 2020-08-09T21:38:47Z

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

jasnell

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 · 2020-08-16T16:58:23Z

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

Thanks

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>

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>

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>

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>

PR-URL: nodejs#34655 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: nodejs#34669 Reviewed-By: Anna Henningsen <anna@addaleax.net>

PR-URL: nodejs#34741 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>

Using a static label is sufficient. PR-URL: nodejs#34741 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: nodejs#34741 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>

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>

PR-URL: nodejs#34741 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>

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>

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>

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>

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>

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>

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 · 2020-08-18T05:07:37Z

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

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>

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>

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 · 2020-08-18T05:12:18Z

Landed in aa5361c...ca5ff72

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>

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>

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>

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>

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>

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 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 requested changes Aug 10, 2020

View reviewed changes

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

ryzokuken approved these changes Aug 17, 2020

View reviewed changes

mildsunrise and others added 4 commits August 17, 2020 13:49

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>

tniessen approved these changes Aug 17, 2020

View reviewed changes

jasnell and others added 18 commits August 17, 2020 11:23

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>

quic: resolve some minor TODOs

34165f0

PR-URL: nodejs#34655 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>

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>

quic: fixup set_socket, fix skipped test

10d5047

PR-URL: nodejs#34669 Reviewed-By: Anna Henningsen <anna@addaleax.net>

quic: check setSocket natRebinding argument, extend test

442968c

PR-URL: nodejs#34669 Reviewed-By: Anna Henningsen <anna@addaleax.net>

quic: add natRebinding argument to docs

c17eaa3

PR-URL: nodejs#34669 Reviewed-By: Anna Henningsen <anna@addaleax.net>

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>

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>

quic: clarify TODO statements

94aa291

PR-URL: nodejs#34741 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>

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>

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>

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>

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>

jasnell approved these changes Aug 18, 2020

View reviewed changes

Trott added 3 commits August 17, 2020 22:09

Trott merged commit ca5ff72 into nodejs:master Aug 18, 2020

Trott deleted the crypto-recommended-1 branch August 18, 2020 05:12

danielleadams mentioned this pull request Aug 20, 2020

V14.9.0 proposal #34852

Merged

codebytere mentioned this pull request Sep 28, 2020

v12.19.0 proposal #35401

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

doc: remove "is recommended" from crypto legacy API text#34697
Trott merged 25 commits intonodejs:masterfrom
Trott:crypto-recommended-1

Trott commented Aug 9, 2020

Uh oh!

jasnell left a comment

Uh oh!

Trott commented Aug 16, 2020

Uh oh!

ryzokuken left a comment

Uh oh!

Trott commented Aug 18, 2020

Uh oh!

Trott commented Aug 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

Uh oh!

Conversation

Trott commented Aug 9, 2020

Checklist

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Aug 16, 2020

Uh oh!

ryzokuken left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Aug 18, 2020

Uh oh!

Trott commented Aug 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants