Skip to content
This repository was archived by the owner on Oct 14, 2024. It is now read-only.

feat(transfer-state): add an interceptor & a service to cache HTTP requests #509

Closed
wants to merge 2 commits into from

Conversation

m98
Copy link
Contributor

@m98 m98 commented Dec 11, 2017

Cache all the HTTP requests on the server by using TransferState API, and return the HTTP response from the cache if exist and it's on the browser and only if it's for the first time.

How does this work

It uses a new interceptor (transfer-state.interceptor) and a service (transfer-state.service) to set, get and manage the cache.

Why is this needed

The first thing user needs it the same ability to just run the app and see the result as he/she expect, and they would think something is broken when they see the flicker caused by the second HTTP request (which is not necessary!) on the browser.
There are lots of different issued filled on this repository, Universal, Angular, Angular-cli, and StackOverflow that developers are looking for an answer, and using a cache mechanism that does this for them seems to be a good idea.
Here are just some of the issues filled for the second HTTP request on the browser:
angular/angular-cli#7477 angular/universal#338 angular/universal#845 angular/angular#20638 #497 #62 #61 #139 #439 #429 #139 angular/universal#436 #7 #57 ...

Thanks to @vikerman this problem can be easily fixed by using TransferState, but it needs some efforts to implement it, and since this repository is a starter-kit for Universal, it's good to have the best practice way to show how to implement TransferState in an Angular app.

@vikerman @Toxicable and @MarkPieszak Can you please let us know your idea of having such a feature in universal-starter?

@vikerman I experienced some strange behavior from TransferState which might be my wrong. But if you think we can have this in universal-starter, we can fix the way I used TransferState

@CaerusKaru
Copy link
Member

CaerusKaru commented Dec 11, 2017

Is there a reason you’re not using the TransferHttpCacheModule from nguniversal/common?

@m98
Copy link
Contributor Author

m98 commented Dec 11, 2017

Because it can be done without adding extra dependency to the project.

@CaerusKaru
Copy link
Member

CaerusKaru commented Dec 11, 2017

That it as an insufficient reason. The whole point of this repository is to provide an example for how to use Universal and its toolchain. The TransferHttpCacheModule is part of this toolchain, just like the express-engine, which is also an "extra dependency". Your solution adds a lot of unnecessary code, and duplicates the rest. Your idea to add HTTP caching to the starter was great, but please either refactor this to use the CacheModule, or close this PR.

If you want to provide an example to users about how to use TransferState, that might be more useful, and could go in an examples directory.

@m98
Copy link
Contributor Author

m98 commented Dec 12, 2017

Thanks for your comment.

Using a dependency does not sound bad, but my idea here is that this repository key point is for users to learn best practices for universal, and other part of Angular.

Using a dependency for this case is fine and it might work very well. But when it comes to having an starter kit which tends to provide good ways for using Angular, and its functionalities, then having your own examples might be more helpful.

Also, already there is not any official example for usingTransferState and this can be a good place to start, and developers can understand interceptors better too.

I am also waiting for other maintainers idea, they might have some different idea with the whole PR

@Toxicable
Copy link

@m98 I agree with @CaerusKaru
We have already implemented this with TransferHttpCacheModule it would be confusing to provide yet another implementation.
That is the recommended solution to this problem

@m98
Copy link
Contributor Author

m98 commented Dec 12, 2017

@Toxicable Thanks for reviewing 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants