IMAGES-1608: Allow base64-encoded input and output in Images Binding #4141
Conversation
| @@ -12,7 +12,26 @@ type ImageInfoResponse = | |||
| }; | |||
|
|
|||
| type ImageTransform = { | |||
| width?: number; | |||
There was a problem hiding this comment.
We had updated types/defines/images.d.ts with more accurate types, but not this file, hence the changes here - the diff on types/defines/images.d.ts is much more reasonable
| * @param {Env} env | ||
| */ | ||
| async test(_, env) { | ||
| const inputs = { |
There was a problem hiding this comment.
Not sure if there is a nice way to unit test the encode/decode transform streams
| @@ -303,6 +327,165 @@ function chainStreams<T>(streams: ReadableStream<T>[]): ReadableStream<T> { | |||
| return outputStream; | |||
| } | |||
|
|
|||
| function concatUint8Arrays(a: Uint8Array, b: Uint8Array): Uint8Array { | |||
There was a problem hiding this comment.
This function doesn't take care of different offsets. Also we can use resizable array buffers, as far as I know, https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/ArrayBuffer/resize
There was a problem hiding this comment.
Holding off on the resizable bit as this would be a more sizeable change and it would be much better to use Buffer instead
| } | ||
|
|
||
| let base64Buffer: Uint8Array | null = null; | ||
| const asciiDecoder = new TextDecoder('ascii'); |
There was a problem hiding this comment.
You can create this in the global scope. No need to create this everytime this method is executed.
There was a problem hiding this comment.
While true, it's generally discouraged to create shared TextDecoder instances because they can contain state depending on what encoding is used and how they are utilized. Keeping this here makes sense I think.
| try { | ||
| const base64SegmentString = asciiDecoder.decode(base64SegmentBytes); | ||
|
|
||
| const binaryString = atob(base64SegmentString); |
There was a problem hiding this comment.
atob is not recommended to be used in production environments. this will throw for utf8 values.
There was a problem hiding this comment.
We probably either need to enable nodejs_compat for this case or expose similar APIs via a cloudflare-internal module.
There was a problem hiding this comment.
I guess as it is base64 we won't see any utf8 values if the base64 is valid, and it throws for invalid base64 anyway? Regardless using the Node APIs sounds way better if doable
| const binaryString = String.fromCharCode.apply(null, Array.from(buf)); | ||
|
|
||
| const base64String = btoa(binaryString); | ||
| const base64Bytes = asciiEncoder.encode(base64String); |
There was a problem hiding this comment.
Hmm... we should probably look at providing a cloudflare-internal:... API to do the base64 encoding more efficiently. If nodejs_compat is on then I we could just use Buffers built-in base64 support. We could expose the same thing via a cloudflare-internal for times when nodejs_compat is not enabled.
| const result = new Uint8Array(a.length + b.length); | ||
| result.set(a, 0); | ||
| result.set(b, a.length); | ||
| return result; |
There was a problem hiding this comment.
This is going to become quite expensive as the buffers get bigger. If nodejs_compat was enabled then we could use Buffer.concat to be far more efficient.
ns476
force-pushed
the
nskehin/base64-handling
branch
from
80975ef to
dd356b8
Compare
ns476
force-pushed
the
nskehin/base64-handling
branch
from
dd356b8 to
e5d6c6e
Compare
No description provided.