refactor(std/node): Make Buffer a third party library #8950
Conversation
| assertEquals(decoder.write(Buffer.from("41F2", "hex")), "8UHy"); | ||
| assertEquals(decoder.end(), ""); | ||
|
|
||
| decoder = new StringDecoder("base64"); | ||
| assertEquals(decoder.text(Buffer.from([0x41]), 2), "QQ=="); | ||
| assertEquals(decoder.text(Buffer.from([0x41]), 2), ""); |
There was a problem hiding this comment.
No opinion on whether it's a good idea to switch implementations but I can confirm that these are indeed the expected outputs.
There was a problem hiding this comment.
Blame it on me, I wrote the original tests :P
| kMaxLength, | ||
| kStringMaxLength, | ||
| SlowBuffer, | ||
| } from "https://esm.sh/[email protected]"; |
There was a problem hiding this comment.
Relying on a third party domain and service here is definitively bit of a no-no from my perspective since std is supposed to be secure and vetted. This extends the trust to an outside source.
Any particular reason we can't just copy this over?
There was a problem hiding this comment.
Importing from an external source in std is not allowed per our style guide (https://deno.land/[email protected]/contributing/style_guide#do-not-depend-on-external-code). If we want to use this implementation, we should copy it over.
|
@lucacasonato What if I wrote an script that copied over the contents of the module and wrote them to a file in std? This would work in this case and would be a way to implement nodejs/readable-stream#452 Plus it would allow us to update the file in case there we needed it to |
Alternative to #8948
This replaces the current implementation of Buffer and puts a reexport of a third-party implementation in the place where it used to be. This enables all the missing features from Buffer and fixes some bugs that seem to be prevalent in the previous implementation (I had to rewrite the tests for string_decoder, since after comparing with the Node implementation they were obviously wrong)