feat(http): allow HttpClient to cache requests
#49509
Conversation
e6051b0 to
8992a71
Compare
8992a71 to
4615d49
Compare
4615d49 to
979c4f7
Compare
979c4f7 to
3014928
Compare
AndrewKushnir
left a comment
There was a problem hiding this comment.
@alan-agius4 looks great! 👍
ba53c71 to
b1ff62a
Compare
As per the discussion and the nice point made by @alan-agius4 regarding the TransferHttpResponse shape under angular/angular#49509. I suggest to align the status of optional and required keys of TransferHttpResponse interface of universal interceptor to meet Angular's packages/common/http/src/transfer_cache.ts counterpart.
As per the discussion and the nice point made by @alan-agius4 regarding the TransferHttpResponse shape under angular/angular#49509. I suggest to align the status of optional and required keys of TransferHttpResponse interface of universal's interceptor to meet Angular's packages/common/http/src/transfer_cache.ts counterpart.
…and headers As per the discussion and the nice point made by @alan-agius4 regarding the TransferHttpResponse shape under angular/angular#49509. I suggest to align the status of optional and required keys of TransferHttpResponse interface of universal's interceptor to meet Angular's packages/common/http/src/transfer_cache.ts counterpart.
…and headers As per the discussion and the nice point made by @alan-agius4 regarding the TransferHttpResponse shape under angular/angular#49509. I suggest to align the status of optional and required keys of TransferHttpResponse interface of universal's interceptor to meet Angular's packages/common/http/src/transfer_cache.ts counterpart.
…and headers As per the discussion and the nice point made by @alan-agius4 regarding the TransferHttpResponse shape under angular/angular#49509. I suggest to align the status of optional and required keys of TransferHttpResponse interface of universal's interceptor to meet Angular's packages/common/http/src/transfer_cache.ts counterpart.
…and headers As per the discussion and the nice point made by @alan-agius4 regarding the TransferHttpResponse shape under angular/angular#49509. I suggest to align the status of optional and required keys of TransferHttpResponse interface of universal's interceptor to meet Angular's packages/common/http/src/transfer_cache.ts counterpart.
…and headers As per the discussion and the nice point made by @alan-agius4 regarding the TransferHttpResponse shape under angular/angular#49509. I suggest to align the status of optional and required keys of TransferHttpResponse interface of universal's interceptor to meet Angular's packages/common/http/src/transfer_cache.ts counterpart.
AndrewKushnir
left a comment
There was a problem hiding this comment.
Looks great! 👍 Just left a few comments.
| const appRef = inject(ApplicationRef); | ||
|
|
||
| let isCacheActive = true; | ||
| appRef.isStable |
There was a problem hiding this comment.
We can also take the InitialRenderPendingTasks status into account as well (similar to utils.ts in the platform-server).
There was a problem hiding this comment.
It also looks like we'd need to create a helper method for this (as a followup), since we use this "signal" in multiple places now.
There was a problem hiding this comment.
We indeed do as there are several places use appRef.isStable were currently InitialRenderPendingTasks is not considered.
I did try to use integrate the usage of InitialRenderPendingTasks but promise was always resolved immediate as collection was empty as I was accessing whenAllTasksComplete in an ENVIRONMENT_INITIALIZER which makes accessing/checking InitialRenderPendingTasks redundant.
I think this is one of the problem that I highlighted previously, that we need to solve.
There was a problem hiding this comment.
We can probably change ENVIRONMENT_INITIALIZER -> APP_BOOTSTRAP_LISTENER, which is used in the hydration code:
https://github.com/angular/angular/blob/main/packages/core/src/hydration/api.ts#L163
In this case, the InitialRenderPendingTasks state would reflect the state of an initial navigation. WDYT?
There was a problem hiding this comment.
Yeah that should work.
b1ff62a to
9ca8a12
Compare
9ca8a12 to
08e71ea
Compare
There was a problem hiding this comment.
Is there a cleaner way?
There was a problem hiding this comment.
Instead of having a default factory, can you just inject it optionally below?
this.injector.get(HTTP_ROOT_INTERCEPTOR_FNS, [])
|
Note: I did not include this as part of a private/public API intentionally until we we finalise the decision on how to consume it. |
…and headers As per the discussion and the nice point made by @alan-agius4 regarding the TransferHttpResponse shape under angular/angular#49509. I suggest to align the status of optional and required keys of TransferHttpResponse interface of universal's interceptor to meet Angular's packages/common/http/src/transfer_cache.ts counterpart.
This commit adds a new option for `provideHttpClient` called `withHttpTransferCache()`. When this option is passed, requests done on the server are cached and reused during the bootstrapping of the application in the browser thus avoiding duplicate requests and reducing load time. This is the same as `TransferHttpCacheModule` in https://github.com/angular/universal/blob/main/modules/common/src/transfer_http.ts
2ec7445 to
d74ab5b
Compare
AndrewKushnir
left a comment
There was a problem hiding this comment.
Looks great, thanks Alan! 👍
|
This PR was merged into the repository by commit aff1512. |
…and headers As per the discussion and the nice point made by @alan-agius4 regarding the TransferHttpResponse shape under angular/angular#49509. I suggest to align the status of optional and required keys of TransferHttpResponse interface of universal's interceptor to meet Angular's packages/common/http/src/transfer_cache.ts counterpart.
…and headers As per the discussion and the nice point made by @alan-agius4 regarding the TransferHttpResponse shape under angular/angular#49509. I suggest to align the status of optional and required keys of TransferHttpResponse interface of universal's interceptor to meet Angular's packages/common/http/src/transfer_cache.ts counterpart. (cherry picked from commit 654ac18)
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
This commit adds a new option for
provideHttpClientcalledwithTransferCache(). When this option is passed, requests done on the server are cached and reused during the bootstrapping of the application in the browser thus avoiding duplicate requests and reducing load time.This is the same as
TransferHttpCacheModulein https://github.com/angular/universal/blob/main/modules/common/src/transfer_http.ts and is part of the effort to move "common" logic that can be used without "@nguniversal/common" into the framework.