Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(platform-browser): enable HTTP request caching when using provideClientHydration #49699

Closed
wants to merge 1 commit into from
Closed

feat(platform-browser): enable HTTP request caching when using provideClientHydration #49699

wants to merge 1 commit into from

Conversation

alan-agius4
Copy link
Contributor

@alan-agius4 alan-agius4 commented Apr 4, 2023 •
edited

This commit adds support by default for HTTP caching when using provideClientHydration. Users can opt-out of this behaviour by using the withoutHttpTransferCache feature.

import {
  bootstrapApplication,
  provideClientHydration,
  withNoHttpTransferCache,
} from '@angular/platform-browser';
// ...
bootstrapApplication(RootCmp, {
  providers: [provideClientHydration(withNoHttpTransferCache())]
});

@pullapprove pullapprove bot requested a review from alxhub April 4, 2023 09:36
@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Apr 4, 2023
alan-agius4
alan-agius4 commented Apr 4, 2023
View reviewed changes
packages/platform-browser/src/hydration.ts Outdated
/**
* Disables DOM nodes reuse during hydration. Effectively makes
* Angular re-render an application from scratch on the client.
*
* @publicApi
* @developerPreview
*/
export function withoutDomReuse(): NoDomReuseFeature {
export function withoutDomReuse(): HydrationFeature<HydrationFeatureKind.NoDomReuseFeature> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are HydrationFeature and HydrationFeatureKind really needed to be part of the public API? In @angular/common/http I noticed they are but was wondering what they are needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I followed the typings that we have in the Router, but I'm ok with this option too 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should they still be exported?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we need to export them.

@alan-agius4 alan-agius4 added this to the v16-candidates milestone Apr 4, 2023
@ngbot ngbot bot removed this from the v16-candidates milestone Apr 4, 2023
@alan-agius4 alan-agius4 added the target: major This PR is targeted for the next major release label Apr 4, 2023
@alan-agius4 alan-agius4 requested review from AndrewKushnir and removed request for alxhub April 4, 2023 09:39
@alan-agius4 alan-agius4 added action: review The PR is still awaiting reviews from at least one requested reviewer area: common Issues related to APIs in the @angular/common package labels Apr 4, 2023
@ngbot ngbot bot added this to the Backlog milestone Apr 4, 2023
@alan-agius4 alan-agius4 removed this from the Backlog milestone Apr 4, 2023
@ngbot ngbot bot added this to the Backlog milestone Apr 4, 2023
@alan-agius4 alan-agius4 modified the milestones: Backlog, v16-candidates Apr 4, 2023
@alan-agius4 alan-agius4 force-pushed the expose-http-cache branch 2 times, most recently from a6ede8b to df6bd17 Compare April 4, 2023 10:04
@alan-agius4
Copy link
Contributor Author

alan-agius4 commented Apr 4, 2023 •
edited

Caretaker note: this requires a G3 patch and BUILD file to be updated. http://cl/521719612

The broken target seems to be caused by other changes in the presubmit CL.

@alan-agius4 alan-agius4 added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Apr 4, 2023
AndrewKushnir
AndrewKushnir reviewed Apr 4, 2023
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

LGTM, just a quick question on the API.

packages/platform-browser/src/hydration.ts Outdated
/**
* Disables DOM nodes reuse during hydration. Effectively makes
* Angular re-render an application from scratch on the client.
*
* @publicApi
* @developerPreview
*/
export function withoutDomReuse(): NoDomReuseFeature {
export function withoutDomReuse(): HydrationFeature<HydrationFeatureKind.NoDomReuseFeature> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I followed the typings that we have in the Router, but I'm ok with this option too 👍

packages/platform-browser/src/hydration.ts Show resolved Hide resolved
@alan-agius4
feat(platform-browser): enable HTTP request caching when using `provi…
9e4365c 
…deClientHydration`

This commit adds support by default for HTTP caching when using `provideClientHydration`. Users can opt-out of this behaviour by using the `withoutHttpTransferCache` feature.

```ts
import {
  bootstrapApplication,
  provideClientHydration,
  withNoHttpTransferCache,
} from '@angular/platform-browser';
// ...
bootstrapApplication(RootCmp, {
  providers: [provideClientHydration(withNoHttpTransferCache())]
});
```
AndrewKushnir
AndrewKushnir approved these changes Apr 4, 2023
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

@alan-agius4 looks great! 👍

@pullapprove pullapprove bot requested review from alxhub and atscott April 4, 2023 18:18
AndrewKushnir
AndrewKushnir approved these changes Apr 4, 2023
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Reviewed-for: public-api

jessicajaniuk
jessicajaniuk approved these changes Apr 4, 2023
View reviewed changes
Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

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

reviewed-for: fw-core, fw-http, fw-platform-server, public-api

@alan-agius4 alan-agius4 removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Apr 4, 2023
@alan-agius4 alan-agius4 added the action: merge The PR is ready for merge by the caretaker label Apr 4, 2023
@AndrewKushnir
Copy link
Contributor

Caretaker notes:

  • this PR is ready for merge
  • this change requires a G3 patch and BUILD file to be updated: http://cl/521719612 (just reposting this from the comment above for visibility)
  • the presubmit is "green" (only unrelated failures)

@dylhunn
Copy link
Contributor

dylhunn commented Apr 4, 2023

This PR was merged into the repository by commit 81e7d15.

@dylhunn dylhunn closed this in 81e7d15 Apr 4, 2023
@alan-agius4 alan-agius4 deleted the expose-http-cache branch April 5, 2023 05:03
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators May 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

@jessicajaniuk jessicajaniuk jessicajaniuk approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: common Issues related to APIs in the @angular/common package detected: feature PR contains a feature commit merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: major This PR is targeted for the next major release
Projects
None yet
Milestone
v16-candidates
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@alan-agius4 @AndrewKushnir @dylhunn @jessicajaniuk