-
Notifications
You must be signed in to change notification settings - Fork 27.1k
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.
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
alan-agius4
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.
LGTM, thanks for this.
packages/common/http/src/util.ts
Outdated
| export function toBase64(buffer: unknown): string { | ||
| const bytes = new Uint8Array(buffer as ArrayBufferLike); |
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.
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.
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.
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.
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.
Is the Uint8Array array empty if we pass it a blob ?
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.
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
bd809c2 to
3fe3545
Compare
| [BODY]: | ||
| req.responseType === 'arraybuffer' || req.responseType === 'blob' | ||
| ? toBase64(event.body) | ||
| ? toBase64(event.body as ArrayBufferLike) |
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.
Is this correct for a blob? If yes we should at least have a test that would show that this is okay.
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.
I agree, I'll add a test for that and validate it.
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.
Can you also add a comment, because that type assertion doesn't make it obvious
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.
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( |
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.
This is necessary because we need to convert asynchronously, and I couldn't think of a better way without adding many more RxJS operators.
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.
The classic RxJs trap. There is no reason why you need RxJs here it just bloats the code and makes it harder to read.
alan-agius4
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.
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( |
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.
The classic RxJs trap. There is no reason why you need RxJs here it just bloats the code and makes it harder to read.
| 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); |
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.
| 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; |
Use feature detection for
Uint8Array.prototype.toBase64andUint8Array.fromBase64, falling back to the existing implementation when native support is not availableThis is considerably faster when we have sizes like 50kb some like
https://github.com/SkyZeroZx/benchmark-encode-decode-64