-
Notifications
You must be signed in to change notification settings - Fork 260
feat(NODE-7314): export byteUtils & add missing methods #852
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
base: main
Are you sure you want to change the base?
Conversation
2379a1e to
b5db818
Compare
nbbeeken
left a comment
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.
Looks great, one concern:
|
|
||
| concat(list: Uint8Array[]): Uint8Array { | ||
| if (list.length === 0) return webByteUtils.allocate(0); | ||
| if (list.length === 1) return list[0]; |
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.
Very clever optimization, but I wonder if returning the input instance could lead to small surprises when BSON is running using this "web mode" implementation.
In Node.js a copy to a new buffer is always performed
baileympearson
left a comment
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.
From the AC in the ticket:
.isBuffer
Does not exist; but we'll either want to remove usages of this from the driver (we'll need to evaluate on a case-by-case basis) or useisUint8Array, which exists in BSON. Maybe it makes sense to export isUint8Array from BSON too.
and
.from
The driver uses Buffer.from to convert the following to buffers:
- utf8 strings
- number arrays
- buffers
- base64 strings
Byte utils supports converting base64 strings and number arrays to Uint8Arrays already. converting buffers to buffers can be done usingisUint8Arrayand conditionally converting the string argument to a buffer if needed. We will need to add conversion of utf8 strings -> buffers to byte utils.
Have you given these APIs thought / are they not needed to remove dependence on the global Buffer object in the driver?
| return 0; | ||
| }, | ||
|
|
||
| concat(list: Uint8Array[]): Uint8Array { |
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.
| concat(list: Uint8Array[]): Uint8Array { | |
| concat(uint8Arrays: Uint8Array[]): Uint8Array { |
or
| concat(list: Uint8Array[]): Uint8Array { | |
| concat(arraysToConcat: Uint8Array[]): Uint8Array { |
or something.
strive for as descriptive variable names as possible
| const result = webByteUtils.allocate(totalLength); | ||
| let offset = 0; | ||
|
|
||
| for (const arr of list) { |
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 (const arr of list) { | |
| for (const uint8Array of uint8Arrays) { |
same as above
|
/*
#ifndef _SYS_DNODE_H #include <grub/zfs/spa.h> /*
/*
#define DNODES_PER_BLOCK_SHIFT (DNODE_BLOCK_SHIFT - DNODE_SHIFT) #define DNODE_FLAG_SPILL_BLKPTR (1<<2) #define DN_BONUS(dnp) ((void*)((dnp)->dn_bonus + typedef struct dnode_phys { } dnode_phys_t; #endif /* _SYS_DNODE_H */ |
Description
Summary of Changes
Added a
compare(a: Uint8Array, b: Uint8Array): -1 | 0 | 1byte utility that performs a lexicographical comparison of two byte arrays.Added a
concat(list: Uint8Array[]): Uint8Arrayutility.And mark
ByteUtilsas public and experimental (similar as forNumberUtils).Notes for Reviewers
compareare intended to match typical lexicographical ordering used for binary data (byte-by-byte from index 0, with shorter equal-prefix arrays considered smaller).concatis implemented using .length on Uint8Array (number of elements), which matches the number of bytes for this typed array and keeps the implementation simple and explicit.What is the motivation for this change?
Release Highlight
Release notes highlight
ByteUtilsare now public and provide set of platform-agnostic tools to manipulate binary data (using Buffer in nodejs-compatible environments and fallback to Uint8Array).Double check the following
npm run check:lint)type(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript