Encode LEB128 into existing arrays

jtenner commented 4 months ago

Hey there! I absolutely love this project, and it's use of zig to make encoding LEB values for wasm very quick. I did find myself really wanting to avoid a heap allocation in the form of a new Uint8Array, so I felt very inspired to generate this particular API to scratch an itch.

Hopefully the following things meet your standards:

Tests: used existing examples to make similar tests to validate value outputs
Implementation: I wanted to make sure I could specify an offset into the destination array, along with keeping the parameter order consistent with the existing decode functions
Error and bounds checking: Using the existing index bounds checker in the error module to make sure the write operation is safe
Documentation example: Copied examples from the template markdown file and added them manually

Notes: I think the prettier command modified a few files, and I added a few missing semicolons, but other than that, everything seems to be done to the best of my ability.

Please let me know if you need anything else for this specific pull request, and I would love to be credited! Thank you for all your hard work.

jtenner commented 3 months ago

Hey any news on this?

jtenner commented 3 months ago

Hey! I'm sorry the comments on the functions weren't well written, but I appreciate the quick acceptance!

postspectacular commented 3 months ago

No worries & apols for not responding sooner with comments - this was a great PR, incl. your comments & even tests! 😍 Thank you very much! I also added you to the package.json & authors.md. Only edited & added to the comments because I realised that one of my older ones was partially factually wrong and I updated the readme example so it makes more sense when extracting the code and running it...

One tiny nitpick (maybe for the future): Your commit message feat(encodeLEB128Into): Encode the LEB bytes into an existing Uint8Array should have used feat(leb128): ... (i.e. the package name). This is kinda important, because a lot of the monorepo tooling (incl. changelog creation & releases), but also other tools are using this information. Mistakes do happen and I thankfully have a system in place to allow aliases (mainly misspellings) for these situations, but these aliases need to be added in multiple places and I just wanted to mention/clarify that the short package name is to be used as context in these commit messages... 👍

thi-ng / umbrella

Encode LEB128 into existing arrays #460