buffer: stricter buffer from #27051
buffer: stricter buffer from #27051
Conversation
BridgeAR
added
the
semver-major
This comment has been minimized.
This comment has been minimized.
nodejs-github-bot
added
the
buffer
This comment has been minimized.
This comment has been minimized.
BridgeAR
force-pushed
the
stricter-buffer-from
branch
from
49a9ca2
to
4284339
Compare
This comment has been minimized.
This comment has been minimized.
|
@nodejs/buffer @nodejs/tsc this could use some reviews |
|
This could use some reviews. |
|
I’m not really convinced by this change. How did we add support for toPrimitive? What was the rationale at the time? |
|
I feel like this is a breaking change that doesn’t give us much in return, so I’d be reluctant to accept it. I think it’s okay to remove the example from the docs as you do here – it’s not something that people would typically want to know, I assume. |
@jasnell added support for it in #13725 due to #13652. There where some minor concerns mentioned about this change as well. The reason why it was implemented seems to be that @addaleax it's the only place in the whole code base where we support this. I personally think it would be best to be consistent and to prevent people from using boxed primitives in general as it's not possible to use them anywhere else either. The implementation also only supports boxed strings - no other wrapped primitive would work. |
We'd have to dig that up, I can't recall now. |
|
The toPrimitive was precisely to handle edge cases as Ruben points out. Wasn't ideal but was needed |
|
Overall I'm fine with the change technically, but similar to @addaleax I'm not sure if there's enough benefit to justify the breaking change. |
|
@jasnell boxed primitives do not use |
|
As I said, it is an edge case :-) ... if there's consensus to pull it out, I'm fine with that so long as the limitation there is documented. |
This validates the byteOffset stricter to prevent ambiguous inputs from being handled in unknown ways.
BridgeAR
force-pushed
the
stricter-buffer-from
branch
from
4284339
to
6f522e3
Compare
So far objects with a `length` property that was not an integer or positive resulted in an empty buffer being created. That is not ideal as it's not really clear what the intention in was. This throws an error from now on instead.
The single buffer API is the only one supporting Symbol.toPrimitive but ideally it should not be used. Remove the documentation to prevent users from potentially using this API.
BridgeAR
force-pushed
the
stricter-buffer-from
branch
from
6f522e3
to
99bdafc
Compare
|
Closing due to lack of continued activity. @BridgeAR, please reopen if/when you plan to pick this back up again |
We have a lot of special casing in
Buffer.fromthat we do not have anywhere else in the code base. There are multiple cases where it's very difficult to actually identify what the users intent was. As such, it seems best to just throw an error instead.@nodejs/buffer this still requires the docs and error messages to be slightly updated but I'd like a first opinion.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes