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

feat(http): add transfer cache module from nguniversal/common #510

Merged
merged 1 commit into from
Feb 13, 2018
Merged

feat(http): add transfer cache module from nguniversal/common #510

merged 1 commit into from
Feb 13, 2018

Conversation

CaerusKaru
Copy link
Member

  • Adds TransferHttpCacheModule from nguniversal/common to avoid
    duplicate HTTP calls between server and browser
  • Bump RxJS dep to 5.5.5 to avoid issues with lower patch versions
  • Clean up certain areas of code, remove redundant imports

PR close #509

@CaerusKaru
Copy link
Member Author

cc @vikerman @Toxicable

@Toxicable
Copy link

Looks good aside from the linting changes, revert those for now.
If you want to fix that up another PR adding a tslint would be great, the default one produce by ng new will do

@CaerusKaru
Copy link
Member Author

@Toxicable on it!

@CaerusKaru
Copy link
Member Author

#511 created to deal with linting

Copy link
Contributor

@m98 m98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job 👍

I think it would be good if you can provide an example of it too
Maybe a new component for HTTP request representing and how does cache work.

@CaerusKaru
Copy link
Member Author

@Toxicable thoughts on @m98's comments? Personally I don't see the usefulness of giving an example for TransferHttpCache since it's pretty self-explanatory, and is mostly here to give a leg-up to developers getting started. But I'm open to adding something if you think it would be useful.

@m98 I still think it would be invaluable if you could provide another example to the repo for demonstrating usage of the TransferState feature, just without HTTP.

@m98
Copy link
Contributor

m98 commented Dec 13, 2017

I did not mean adding an example to demonstrating usage of the TransferState

But since this pull request adds ability to transfer state from server to browser, it might be helpful to use a simple HTTP request just let users know there is already something which is supporting transfer state.

They might write their own services to support this if they don't know such thing added to the repo

@Toxicable
Copy link

I think adding an example for each would be the better path for this one, so that we can compare and show the different situations you'd use either

.gitignore Outdated
@@ -2,6 +2,7 @@
.DS_Store
.vscode
morgan.log
package-lock.json

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uhhhh we shouldn't be ignoring this file.
We should either use Yarn or Npm and stick to just one.
I vote Yarn :P

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

package.json Outdated
"@nguniversal/express-engine": "^5.0.0-beta.5",
"@nguniversal/module-map-ngfactory-loader": "^5.0.0-beta.5",
"core-js": "^2.4.1",
"rxjs": "^5.5.2",
"rxjs": "^5.5.5",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change is unneeded

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

* Adds TransferHttpCacheModule from nguniversal/common to avoid
  duplicate HTTP calls between server and browser
* Bump RxJS dep to 5.5.5 to avoid issues with lower patch versions
@Toxicable Toxicable merged commit 0b3788c into angular:master Feb 13, 2018
@CaerusKaru CaerusKaru deleted the http-cache branch February 13, 2018 00:03
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