Skip to content

Conversation

@SkyZeroZx
Copy link
Contributor

Use feature detection for Uint8Array.prototype.toBase64 and Uint8Array.fromBase64, falling back to the existing implementation when native support is not available

This is considerably faster when we have sizes like 50kb some like

https://github.com/SkyZeroZx/benchmark-encode-decode-64

encode workaround x 3,732 ops/sec ±2.72% (92 runs sampled)
encode native x 232,185 ops/sec ±1.93% (91 runs sampled)
decode workaround x 258 ops/sec ±2.56% (88 runs sampled)
decode native x 39,383 ops/sec ±1.92% (89 runs sampled)

@pullapprove pullapprove bot requested a review from AndrewKushnir February 10, 2026 18:31
@angular-robot angular-robot bot added the area: common/http Issues related to HTTP and HTTP Client label Feb 10, 2026
@ngbot ngbot bot added this to the Backlog milestone Feb 10, 2026
Copy link
Contributor

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for this.

@alan-agius4 alan-agius4 added target: patch This PR is targeted for the next patch release action: merge The PR is ready for merge by the caretaker action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: merge The PR is ready for merge by the caretaker labels Feb 11, 2026
Comment on lines 23 to 24
export function toBase64(buffer: unknown): string {
const bytes = new Uint8Array(buffer as ArrayBufferLike);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks off. Since toBase64 can get a Blob would it throw ?

I would very much prefer that this function is correctly typed and that the type assertion is made in the transfer cache.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve just updated it. It’s weird, but it doesn’t throw an exception if we pass a Blob; it just sets the Uint8Array length to 0.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the Uint8Array array empty if we pass it a blob ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can pass some like

const myBlob = new Blob(['hello' , 'world'])
const u8 = new Uint8Array(myBlob)
console.log(u8)

Use feature detection for `Uint8Array.prototype.toBase64` and
`Uint8Array.fromBase64`, falling back to the existing implementation
when native support is not available
@SkyZeroZx SkyZeroZx force-pushed the refactor/http-from-to-64 branch from bd809c2 to 3fe3545 Compare February 11, 2026 16:59
[BODY]:
req.responseType === 'arraybuffer' || req.responseType === 'blob'
? toBase64(event.body)
? toBase64(event.body as ArrayBufferLike)
Copy link
Member

@JeanMeche JeanMeche Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct for a blob? If yes we should at least have a test that would show that this is okay.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I'll add a test for that and validate it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add a comment, because that type assertion doesn't make it obvious

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct for a blob? If yes we should at least have a test that would show that this is okay.

I added a fix in this PR because parsing was needed. I tested it with Protocol Buffer, and I was confused by the response because it was always a Uint8Array (which is curious, since it works fine when setting a Blob). That was misleading on my part. This should fix the issue, and I tested both cases ( Blob and Uint8Array ) to ensure consistency

Previously, Blob values were passed to `Uint8Array` this resulted in silently producing an empty array (length = 0) without throwing an error, leading to empty cached data
// Note: Blob is converted to ArrayBuffer because Uint8Array constructor
// doesn't accept Blob directly, which would result in an empty array.
// Type assertion is safe here: when responseType is 'blob', the body is guaranteed to be a Blob
return from((event.body as Blob).arrayBuffer()).pipe(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is necessary because we need to convert asynchronously, and I couldn't think of a better way without adding many more RxJS operators.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The classic RxJs trap. There is no reason why you need RxJs here it just bloats the code and makes it harder to read.

Copy link
Contributor

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments

// Note: Blob is converted to ArrayBuffer because Uint8Array constructor
// doesn't accept Blob directly, which would result in an empty array.
// Type assertion is safe here: when responseType is 'blob', the body is guaranteed to be a Blob
return from((event.body as Blob).arrayBuffer()).pipe(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The classic RxJs trap. There is no reason why you need RxJs here it just bloats the code and makes it harder to read.

Comment on lines +226 to +260
concatMap((event: HttpEvent<unknown>) => {
// Only cache successful HTTP responses.
if (event instanceof HttpResponse) {
transferState.set<TransferHttpResponse>(storeKey, {
[BODY]:
req.responseType === 'arraybuffer' || req.responseType === 'blob'
? toBase64(event.body)
: event.body,
[HEADERS]: getFilteredHeaders(event.headers, headersToInclude),
[STATUS]: event.status,
[STATUS_TEXT]: event.statusText,
[REQ_URL]: requestUrl,
[RESPONSE_TYPE]: req.responseType,
});
const storeInCache = (body: unknown) => {
transferState.set<TransferHttpResponse>(storeKey, {
[BODY]: body,
[HEADERS]: getFilteredHeaders(event.headers, headersToInclude),
[STATUS]: event.status,
[STATUS_TEXT]: event.statusText,
[REQ_URL]: requestUrl,
[RESPONSE_TYPE]: req.responseType,
});
};

if (req.responseType === 'blob') {
// Convert Blob to ArrayBuffer asynchronously before caching.
// Note: Blob is converted to ArrayBuffer because Uint8Array constructor
// doesn't accept Blob directly, which would result in an empty array.
// Type assertion is safe here: when responseType is 'blob', the body is guaranteed to be a Blob
return from((event.body as Blob).arrayBuffer()).pipe(
tap((arrayBuffer) => storeInCache(toBase64(arrayBuffer))),
concatMap(() => of(event)),
);
}

// For arraybuffer, convert to base64; for other types (json, text), store as-is.
// Type assertion is safe here: when responseType is 'arraybuffer', the body is
// guaranteed to be an ArrayBuffer
const body =
req.responseType === 'arraybuffer'
? toBase64(event.body as ArrayBufferLike)
: event.body;
storeInCache(body);
}
return of(event);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
concatMap((event: HttpEvent<unknown>) => {
// Only cache successful HTTP responses.
if (event instanceof HttpResponse) {
transferState.set<TransferHttpResponse>(storeKey, {
[BODY]:
req.responseType === 'arraybuffer' || req.responseType === 'blob'
? toBase64(event.body)
: event.body,
[HEADERS]: getFilteredHeaders(event.headers, headersToInclude),
[STATUS]: event.status,
[STATUS_TEXT]: event.statusText,
[REQ_URL]: requestUrl,
[RESPONSE_TYPE]: req.responseType,
});
const storeInCache = (body: unknown) => {
transferState.set<TransferHttpResponse>(storeKey, {
[BODY]: body,
[HEADERS]: getFilteredHeaders(event.headers, headersToInclude),
[STATUS]: event.status,
[STATUS_TEXT]: event.statusText,
[REQ_URL]: requestUrl,
[RESPONSE_TYPE]: req.responseType,
});
};
if (req.responseType === 'blob') {
// Convert Blob to ArrayBuffer asynchronously before caching.
// Note: Blob is converted to ArrayBuffer because Uint8Array constructor
// doesn't accept Blob directly, which would result in an empty array.
// Type assertion is safe here: when responseType is 'blob', the body is guaranteed to be a Blob
return from((event.body as Blob).arrayBuffer()).pipe(
tap((arrayBuffer) => storeInCache(toBase64(arrayBuffer))),
concatMap(() => of(event)),
);
}
// For arraybuffer, convert to base64; for other types (json, text), store as-is.
// Type assertion is safe here: when responseType is 'arraybuffer', the body is
// guaranteed to be an ArrayBuffer
const body =
req.responseType === 'arraybuffer'
? toBase64(event.body as ArrayBufferLike)
: event.body;
storeInCache(body);
}
return of(event);
concatMap(async (event: HttpEvent<unknown>) => {
// Only cache successful HTTP responses.
if (event instanceof HttpResponse) {
let body = event.body;
if (req.responseType === 'blob') {
const arrayBuffer = await (event.body as Blob).arrayBuffer();
body = toBase64(arrayBuffer);
} else if (req.responseType === 'arraybuffer') {
body = toBase64(event.body as ArrayBufferLike);
}
transferState.set<TransferHttpResponse>(storeKey, {
[BODY]: body,
[HEADERS]: getFilteredHeaders(event.headers, headersToInclude),
[STATUS]: event.status,
[STATUS_TEXT]: event.statusText,
[REQ_URL]: requestUrl,
[RESPONSE_TYPE]: req.responseType,
});
}
return event;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

action: review The PR is still awaiting reviews from at least one requested reviewer area: common/http Issues related to HTTP and HTTP Client target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants