-
Notifications
You must be signed in to change notification settings - Fork 30k
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.
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
doc: clarify behavior of byteLength with 'base64' #11238
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of nits but otherwise LGTM
doc/api/buffer.md
Outdated
@@ -618,6 +618,10 @@ Returns the actual byte length of a string. This is not the same as | |||
[`String.prototype.length`] since that returns the number of *characters* in | |||
a string. | |||
|
|||
_Note_ that for `'base64'`, this function assumes valid input. For strings that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, please s/_Note_/*Note*:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn’t specific to base64
, it applies at least to hex
, too:
> Buffer.byteLength('aa ', 'hex')
2
> Buffer.from('aa ', 'hex').length
1
(I think it’s only those two encodings but I didn’t double-check.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@addaleax oh indeed. That makes wording it a bit more complicated...
doc/api/buffer.md
Outdated
@@ -618,6 +618,10 @@ Returns the actual byte length of a string. This is not the same as | |||
[`String.prototype.length`] since that returns the number of *characters* in | |||
a string. | |||
|
|||
_Note_ that for `'base64'`, this function assumes valid input. For strings that | |||
aren't valid Base64, the return value might be greater than the length of a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/aren't/are not
doc/api/buffer.md
Outdated
@@ -618,6 +618,10 @@ Returns the actual byte length of a string. This is not the same as | |||
[`String.prototype.length`] since that returns the number of *characters* in | |||
a string. | |||
|
|||
*Note* that for `'base64'` and `'hex'`, this function assumes valid input. For | |||
strings that do not contain Base64/Hex-encoded data, the return value might be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Maybe mention that the method doesn’t account for whitespace in the input … for some people that might be included in “invalid”, but that’s kind of subjective…
Done, but now I have trouble coming up with a descriptive commit message. Any better suggestions? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
“doc: clarify the behavior of byteLength” (or “doc: clarify the behavior of Buffer.byteLength” if you feel like it) sounds perfectly fine to me :)
@@ -618,6 +618,10 @@ Returns the actual byte length of a string. This is not the same as | |||
[`String.prototype.length`] since that returns the number of *characters* in | |||
a string. | |||
|
|||
*Note* that for `'base64'` and `'hex'`, this function assumes valid input. For | |||
strings that contain non-Base64/Hex-encoded data (e.g. whitespace), the return | |||
value might be greater than the length of a `Buffer` created from the string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return value could be smaller too (e.g. Buffer.byteLength('1345 4 ', 'hex') === 3
, Buffer.from('1345 4 ').byteLength === 7
), but then since the data is invalid it's just hard to say. Maybe just say something like the return value is meaningless and should not be used
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, wait, I forgot to pass hex
to Buffer.from('1345 4 ')
, that one would just throws TypeError: Invalid hex string
, so there is nothing to compare :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, logically there is no way for it to be smaller. That would imply that you can encode more data by using an invalid string than a valid string.
It's not completely meaningless - e.g. if you allocate a Buffer with the size at least Buffer.byteLength, then the actual buffer will fit in it. Although I can't imagine a use case for it.
PR-URL: nodejs#11238 Refs: nodejs#11165 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com>
PR-URL: nodejs#11238 Refs: nodejs#11165 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com>
Ref: #11165
Checklist
Affected core subsystem(s)
doc