feat(common): add a common module with TransferHttpCacheModule #823
Conversation
modules/common/src/transfer_http.ts
Outdated
| @@ -0,0 +1,105 @@ | |||
| import 'rxjs/add/observable/of'; | |||
There was a problem hiding this comment.
any reason to not replace this with of import rather than modifying the prototype?
There was a problem hiding this comment.
Hmm - This seems to be the canonical way to include Observable.of. What did you have in mind?
There was a problem hiding this comment.
import {of } from 'rxjs/observable/of';
see here for example https://github.com/angular/angular/blob/3a227a1f6fb9d734aeba83cdf80246ae775a4f53/packages/router/src/utils/collection.ts#L12
modules/common/src/transfer_http.ts
Outdated
| @Inject(PLATFORM_ID) private platformId: Object) { | ||
| // Stop using the cache if the application has stabilized, indicating initial rendering is | ||
| // complete. | ||
| toPromise |
There was a problem hiding this comment.
Shouldn't this be wrapped in a if(isPlatformBrowser())?
There was a problem hiding this comment.
No. Both the server and client stop after initial render.
There was a problem hiding this comment.
Then wouldn't it be better to use the BEFORE_APP_SERIALIZED token instead of onStable on the server?
There was a problem hiding this comment.
The idea is that the cache is active only for the time of the initial render - Initial render is done when the app has become stable the first time.
Currently we don't want to be in the business of a generic HTTP cache past the initial render. But will decide it based on how people want to use it.
There was a problem hiding this comment.
ah right, that makes sense, also means slightly less code which is always nice
|
Glad this will become a common part of the npm package.. I'd recommend some tests.. We had a large amount of bugs in our app until we wrote a few tests for the http transfer interceptors and traced the system. |
|
@patrickmichalina We don't currently have a testing suite setup but once we do all modules will have tests added. |
|
@patrickmichalina - Totally agreed on tests. I am in the process of setting up the tests in the CI - But wanted to get this out meanwhile. The tests are in here when I created this PR originally for angular/angular - But we decided the http cache was too experimental to put it in core right now - vikerman/angular@3e2979e#diff-2fe0b6298256d163bcf56eb631187740 . I will get the tests ported in here. |
|
Is it available for aspnetcore-engine as well? |
|
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. |
Add a TransferHttpCacheModule that when included in the App module will start caching Http requests on the server and transfer the cache to the client.
This will help avoid duplicate requests from the client for requests that the server application already made.