doc: add tls.convertALPNProtocols(protocols, out) #58026
Conversation
nodejs-github-bot
added
doc
Add convertALPNProtocols() to doc/api/tls.md, since the source code actually exposes this method. Refs: nodejs@802a2e7
hkleungai
force-pushed
the
doc-api-tls-add-convertALPNProtocols
branch
from
1fb4791 to
eb1564f
Compare
|
See if maintainer want to approve and merge this doc change, or if it is actually better to move this unmentioned method into |
|
Cc @nodejs/tsc, typescript folks doesn't want to includes types for undocumented nodejs functions, but without the types, these methods require ts override to be callable, which is not ideal. |
Moving to internal would be a breaking change and requires a deprecation cycle. |
|
Seems this doc change cause the test failed in unrelated weird place. See if maintainers have any idea about it. === release test-zlib-convenience-methods ===
Path: parallel/test-zlib-convenience-methods
Error: Command: out/Release/node --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout "/home/runner/work/node/node/dir%20with $unusual"chars?'åß∂ƒ©∆¬…`/test/parallel/test-zlib-convenience-methods.js"
--- TIMEOUT ---
===
=== 1 tests failed
===
Failed tests:
out/Release/node --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout "/home/runner/work/node/node/dir%20with $unusual"chars?'åß∂ƒ©∆¬…`/test/parallel/test-zlib-convenience-methods.js"
Error: Process completed with exit code 1. |
anonrig
added
the
author ready
There was a problem hiding this comment.
I don't think this was intended to be exposed for public consumption.
The whole point of this function is to be able to convert user supplied protocols into the wire-format supported by openssl: https://docs.openssl.org/master/man3/SSL_CTX_set_alpn_select_cb/#notes. This is already being done internally everywhere in Node.js where protocol inputs are being accepted.
I'm not able to think of any reason how this function would be useful outside of Node.js core code. I also tried to search for uses in https://github.com/search?q=tls.convertALPNProtocols&type=code and I wasn't able to find any use outside of Node.js core code.
I would be +1 about deprecating this function and using an internal version of it in Node.js. Adding an entry in https://nodejs.org/api/deprecations.html is a must for doing that.
| `['http/1.1', 'http/1.0']`. Protocols earlier in the list have higher | ||
| preference than those later. | ||
| * `out` {Object} A output variable supplied by users, in which the converted `protocols` result `out.ALPNProtocols`. | ||
|
|
There was a problem hiding this comment.
There is usually a section documenting the function after the prototype documentation. That needs to be added if we want to document this function as a part of our public API.
| next protocol name. Passing an array is usually much simpler, e.g. | ||
| `['http/1.1', 'http/1.0']`. Protocols earlier in the list have higher | ||
| preference than those later. | ||
| * `out` {Object} A output variable supplied by users, in which the converted `protocols` result `out.ALPNProtocols`. |
There was a problem hiding this comment.
| * `out` {Object} A output variable supplied by users, in which the converted `protocols` result `out.ALPNProtocols`. | |
| * `out` {Object} An output variable supplied by users. The converted `protocols` result is assigned to `out.ALPNProtocols`. |
Add convertALPNProtocols() to doc/api/tls.md, since the source code
actually exposes this method.
Refs: 802a2e7