feat(http): allow HttpClient to cache requests
#49509
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alan-agius4
force-pushed
the
transfer-http
branch
from
e6051b0
to
8992a71
Compare
alan-agius4
force-pushed
the
transfer-http
branch
from
8992a71
to
4615d49
Compare
alan-agius4
force-pushed
the
transfer-http
branch
from
4615d49
to
979c4f7
Compare
alan-agius4
commented
Mar 21, 2023
alan-agius4
added
the
target: minor
alan-agius4
force-pushed
the
transfer-http
branch
from
979c4f7
to
3014928
Compare
alan-agius4
added
the
action: review
AndrewKushnir
approved these changes
Mar 21, 2023
atscott
reviewed
Mar 22, 2023
bougwal
reviewed
Mar 22, 2023
alan-agius4
force-pushed
the
transfer-http
branch
5 times, most recently
from
ba53c71
to
b1ff62a
Compare
bougwal
added a commit
to bougwal/universal
that referenced
this pull request
Mar 22, 2023
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.
bougwal
pushed a commit
to bougwal/universal
that referenced
this pull request
Mar 22, 2023
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.
bougwal
pushed a commit
to bougwal/universal
that referenced
this pull request
Mar 22, 2023
…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.
bougwal
pushed a commit
to bougwal/universal
that referenced
this pull request
Mar 23, 2023
…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.
bougwal
added a commit
to bougwal/universal
that referenced
this pull request
Mar 23, 2023
…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.
bougwal
added a commit
to bougwal/universal
that referenced
this pull request
Mar 23, 2023
…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.
bougwal
added a commit
to bougwal/universal
that referenced
this pull request
Mar 24, 2023
…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.
| 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.
There was a problem hiding this comment.
@AndrewKushnir PTAL
alan-agius4
force-pushed
the
transfer-http
branch
from
b1ff62a
to
9ca8a12
Compare
alan-agius4
force-pushed
the
transfer-http
branch
from
9ca8a12
to
08e71ea
Compare
alan-agius4
commented
Mar 31, 2023
| /** | ||
| * A multi-provided token of `HttpInterceptorFn`s that are only set in root. | ||
| */ | ||
| export const HTTP_ROOT_INTERCEPTOR_FNS = new InjectionToken<HttpInterceptorFn[]>( |
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, [])
alan-agius4
added
the
action: review
atscott
approved these changes
Mar 31, 2023
|
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. |
bougwal
added a commit
to bougwal/universal
that referenced
this pull request
Apr 1, 2023
…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
alan-agius4
force-pushed
the
transfer-http
branch
from
2ec7445
to
d74ab5b
Compare
AndrewKushnir
approved these changes
Apr 3, 2023
AndrewKushnir
removed
the
action: review
|
This PR was merged into the repository by commit aff1512. |
alan-agius4
pushed a commit
to angular/universal
that referenced
this pull request
Apr 12, 2023
…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.
alan-agius4
pushed a commit
to angular/universal
that referenced
this pull request
Apr 12, 2023
…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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.