Skip to content

refactor(http): Improves base64 encoding/decoding with feature detection #67002

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.

Sign up for GitHub

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

Open
SkyZeroZx wants to merge 5 commits into
base: main
Choose a base branch
from
+209 −152
Open

refactor(http): Improves base64 encoding/decoding with feature detection #67002

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2acd6ad
fix(http): correctly cache blob responses in transfer cache
SkyZeroZx Feb 11, 2026
6a0a9ed
fixup! fix(http): correctly cache blob responses in transfer cache
SkyZeroZx Feb 12, 2026
e9eaf18
fixup! fix(http): correctly cache blob responses in transfer cache
SkyZeroZx Feb 13, 2026
63f083d
fixup! fix(http): correctly cache blob responses in transfer cache
SkyZeroZx Feb 18, 2026
8312083
test(http): correct response handling in httpResource
SkyZeroZx Mar 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 17 additions & 6 deletions packages/common/http/src/transfer_cache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import {
ɵRuntimeError as RuntimeError,
} from '@angular/core';
import {Observable, of} from 'rxjs';
import {tap} from 'rxjs/operators';
import {concatMap} from 'rxjs/operators';

import {RuntimeErrorCode} from './errors';
import {HttpHeaders} from './headers';
Expand Down Expand Up @@ -266,21 +266,32 @@ export function transferCacheInterceptorFn(
if (typeof ngServerMode !== 'undefined' && ngServerMode) {
// Request not found in cache. Make the request and cache it if on the server.
return event$.pipe(
tap((event: HttpEvent<unknown>) => {
concatMap(async (event: HttpEvent<unknown>) => {
// Only cache successful HTTP responses.
if (event instanceof HttpResponse) {
let body = event.body;
if (req.responseType === 'blob') {
// 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
const arrayBuffer = await (event.body as Blob).arrayBuffer();
body = toBase64(arrayBuffer);
} else if (req.responseType === 'arraybuffer') {
// 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
body = toBase64(event.body as ArrayBufferLike);
}
transferState.set<TransferHttpResponse>(storeKey, {
[BODY]:
req.responseType === 'arraybuffer' || req.responseType === 'blob'
? toBase64(event.body)
: event.body,
[BODY]: body,
[HEADERS]: getFilteredHeaders(event.headers, headersToInclude),
[STATUS]: event.status,
[STATUS_TEXT]: event.statusText,
[REQ_URL]: requestUrl,
[RESPONSE_TYPE]: req.responseType,
});
}
return event;
}),
);
}
Expand Down
12 changes: 4 additions & 8 deletions packages/common/http/test/resource_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
import {HttpTestingController, provideHttpClientTesting} from '../testing';
import {withHttpTransferCache} from '../src/transfer_cache';
import {HttpClient} from '../src/client';
import {lastValueFrom} from 'rxjs';

describe('httpResource', () => {
beforeEach(() => {
Expand Down Expand Up @@ -424,15 +425,10 @@ describe('httpResource', () => {

it('should synchronously resolve with a cached value from TransferState', async () => {
Copy link
Contributor Author

@SkyZeroZx SkyZeroZx Mar 4, 2026

Choose a reason for hiding this comment

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

According to my review, this test was failing due to the recent changes to the transfer cache, which now behaves asynchronously.
I'm not sure if we should change the test name or if it's correct.

Copy link
Contributor

@thePunderWoman thePunderWoman Mar 4, 2026

Choose a reason for hiding this comment

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

This change was intentional to ensure there's no loading state when pulling data from the transfer cache. #67307

Copy link
Contributor Author

@SkyZeroZx SkyZeroZx Mar 4, 2026

Choose a reason for hiding this comment

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

So I think it would stay as it is, only keeping the change I made due to the TransferCache update

Copy link
Contributor Author

@SkyZeroZx SkyZeroZx Mar 13, 2026

Choose a reason for hiding this comment

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

@thePunderWoman With the latest changes, I think it would be okay to merge, or would we still need to adjust something?

globalThis['ngServerMode'] = true;
let requestResolved = false;
TestBed.inject(HttpClient)
.get('/data')
.subscribe(() => (requestResolved = true));
const req = TestBed.inject(HttpTestingController).expectOne('/data');
req.flush([1, 2, 3]);

expect(requestResolved).toBe(true);

const response = lastValueFrom(TestBed.inject(HttpClient).get('/data'));
TestBed.inject(HttpTestingController).expectOne('/data').flush([1, 2, 3]);
await response;
// Now switch to client mode
globalThis['ngServerMode'] = false;

Expand Down
Loading