http2: stricter settings validation #26811
Conversation
nodejs-github-bot
added
dont-land-on-v6.x
http2
The function did not only validate the input so far but it also made a copy of the input object and returned that copy to the callee function. That copy was not necessary for all call sites and it was not obvious that the function did not only validate the input but that it also returned a copy of it. This makes sure the function does nothing more than validation and copying is happening in the callee function when required.
The settings input was not marked as optional in our documentation but we did not throw an error in those cases. The API does not seem to be useful without the correct input, so I went for throwing an error in such cases instead of updating the documentation.
BridgeAR
force-pushed
the
stricter-http2-settings-validation
branch
from
11e3ca6
to
5c1b27f
Compare
BridgeAR
added
the
semver-major
There was a problem hiding this comment.
Allowing settings to be undefined was intentional. Calling http2.getPackedSettings() with an undefined argument and getting back an empty buffer is perfectly legitimate as it would imply that the defaults would be used, or that no actual change to the settings is being made.
|
@jasnell I am not sure I can follow. The return value will be empty in such case and it is just a |
|
I'm not saying the function cannot be improved, I'm saying that throwing an error if the input is undefined is wrong. It should be perfectly fine is pass in |
|
@jasnell since I am on this anyway: what would be the ideal behavior instead? The current behavior does not seem to benefit anyone, so I could just go ahead and implement what you think should be done instead. |
|
As I said, it should be perfectly fine to pass in settings as |
|
That is currently possible, it's just a no-op right now. Should something specific be returned instead of an empty string? |
|
It should match the current behavior.... Returning an empty Buffer
…On Fri, Mar 22, 2019, 08:48 Ruben Bridgewater ***@***.***> wrote:
That is currently possible, it's just a no-op right now. Should something
specific be returned instead of an empty string?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#26811 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAa2eaDbk4erK1iN2uVJdedmag-Kewziks5vZPtQgaJpZM4b_O3r>
.
|
The settings are currently not required and the callback was not documented so far. Refs: nodejs#26811
|
Closing in favor of #26894 |
The settings are currently not required and the callback was not documented so far. Refs: nodejs#26811 PR-URL: nodejs#26894 Refs: nodejs#26811 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
The settings are currently not required and the callback was not documented so far. Refs: nodejs#26811 PR-URL: nodejs#26894 Refs: nodejs#26811 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
This relies upon #26809 and should not yet be merged. I just wanted to open the PR to get it out early.Update: It is actually not important in what order these PRs land. I'll update what ever comes first.
// cc @nodejs/http2
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes