crypto: extract throwInvalidArgType function #22947
Conversation
|
Not opposed to this change, but the name |
Yeah, I agree. I was struggling to come up with a good name. Let me try to come up with a better name for the function. |
lib/internal/crypto/cipher.js
Outdated
There was a problem hiding this comment.
This change seems like a mistake. The toBuf() function accepts a string and transform that into a buffer.
There was a problem hiding this comment.
The motivation for this changes was that toBuf() would convert any string passed in to a Buffer, and therefore isArrayBufferView would not be passed a string for the password, key, and iv cases.
Note that my first commit also updated the update function which does not call toBuf() which was a mistake and this as fixed in the latest commit, so update will also include the string in its throws cause.
Does that still sound incorrect?
There was a problem hiding this comment.
The error message corresponds to the possible values for the user, not to possible transformations we do internally before throwing the error. So string should still be valid in all of these cases.
lib/internal/crypto/cipher.js
Outdated
There was a problem hiding this comment.
Nit: I would prefer constructing the error object in the function and throwing it explicitly here. I think it would be better during the reviews and one less line in the stack trace 😉.
There was a problem hiding this comment.
That makes sense. I'll update shortly. Thanks!
|
Re-run of failing node-test-commit-smartos and node-test-commit-linux. |
This commit extracts the throwing of ERR_INVALID_ARG_TYPE which is done in identical ways in a few places in cipher.js. The motivation for this is that I think it improves readability enough to warrant a commit even though I'm aware that we should avoid commits with only these sort of refactoring.
0c53f0e to
310ed4f
Compare
|
Re-run of failing node-test-commit-arm |
|
Landed in b25e6ab. |
This commit extracts the throwing of ERR_INVALID_ARG_TYPE which is done in identical ways in a few places in cipher.js. The motivation for this is that I think it improves readability enough to warrant a commit even though I'm aware that we should avoid commits with only these sort of refactoring. PR-URL: #22947 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
This commit extracts the throwing of ERR_INVALID_ARG_TYPE which is done in identical ways in a few places in cipher.js. The motivation for this is that I think it improves readability enough to warrant a commit even though I'm aware that we should avoid commits with only these sort of refactoring. PR-URL: #22947 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
This commit extracts the throwing of ERR_INVALID_ARG_TYPE which is done in identical ways in a few places in cipher.js. The motivation for this is that I think it improves readability enough to warrant a commit even though I'm aware that we should avoid commits with only these sort of refactoring. PR-URL: #22947 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
This commit extracts the throwing of ERR_INVALID_ARG_TYPE which is done
in identical ways in a few places in cipher.js.
The motivation for this is that I think it improves readability enough to
warrant a commit even though I'm aware that we should avoid commits with
only these sort of refactoring.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes