fix(http): include transferCache when cloning HttpRequest #54939

mlz11 · 2024-03-19T07:54:44Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/main/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Fixes #54924.

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

Other information

packages/common/http/src/request.ts

alan-agius4

Thanks for this.

Can you please update the commit message scope from common to http?

mlz11 · 2024-03-19T10:26:05Z

Thanks for this.

Can you please update the commit message scope from common to http?

Thanks @alan-agius4 for the quick review 🙂
Of course, done ✅

alan-agius4

LGTM, thanks for your contribution

alan-agius4 · 2024-03-19T10:55:01Z

Can you please run update the golden files via yarn bazel run //packages/common:common_api.accept?

Fixes a bug where HttpRequest.clone() does not include the transferCache property. Fixes angular#54924.

Refactor how boolean options are handled in HttpRequest.clone() method by using nullish coalescing operator

mlz11 · 2024-03-19T10:57:59Z

Can you please run update the golden files via yarn bazel run //packages/common:common_api.accept?

Of course, done ✅
I was wondering why tests were failing 😅

kbrilla · 2024-03-19T11:07:08Z

packages/common/http/src/request.ts

@@ -479,6 +482,10 @@ export class HttpRequest<T> {
    const url = update.url || this.url;
    const responseType = update.responseType || this.responseType;

+    // Carefully handle the transferCache to differentiate between
+    // `false` and `undefined` in the update args.


now it also handles null so mayby add that to comment : undefined or null here and in other places that were edited. Also why not add move this logic together with withCredentials and reportProgress? becouse now
this comment is duplicated in lines 485 and 495

alan-agius4

LGTM

dylhunn

reviewed-for: public-api

jessicajaniuk

reviewed-for: public-api

atscott

reviewed-for: public-api

dylhunn · 2024-03-22T05:13:49Z

This PR was merged into the repository by commit b80434a.

#54939) Refactor how boolean options are handled in HttpRequest.clone() method by using nullish coalescing operator PR Close #54939

Fixes a bug where HttpRequest.clone() does not include the transferCache property. Fixes #54924. PR Close #54939

#54939) Refactor how boolean options are handled in HttpRequest.clone() method by using nullish coalescing operator PR Close #54939

) Fixes a bug where HttpRequest.clone() does not include the transferCache property. Fixes angular#54924. PR Close angular#54939

angular#54939) Refactor how boolean options are handled in HttpRequest.clone() method by using nullish coalescing operator PR Close angular#54939

angular-automatic-lock-bot · 2024-04-22T00:09:35Z

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.}

pullapprove bot requested a review from dylhunn March 19, 2024 07:54

mlz11 force-pushed the include-transferCache-in-http-request-clone-method branch from b07fbb4 to f829824 Compare March 19, 2024 07:58

mlz11 mentioned this pull request Mar 19, 2024

HttpRequest.clone forgets transferCache #54924

Closed

mlz11 force-pushed the include-transferCache-in-http-request-clone-method branch 4 times, most recently from 6b9a647 to 12b50df Compare March 19, 2024 08:24

alan-agius4 reviewed Mar 19, 2024

View reviewed changes

packages/common/http/src/request.ts Outdated Show resolved Hide resolved

alan-agius4 requested changes Mar 19, 2024

View reviewed changes

alan-agius4 added area: common/http target: patch This PR is targeted for the next patch release server: http cache labels Mar 19, 2024

ngbot bot added this to the Backlog milestone Mar 19, 2024

alan-agius4 added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Mar 19, 2024

mlz11 force-pushed the include-transferCache-in-http-request-clone-method branch from 12b50df to 6f244fc Compare March 19, 2024 10:25

mlz11 force-pushed the include-transferCache-in-http-request-clone-method branch from 6f244fc to 8a90bb8 Compare March 19, 2024 10:30

alan-agius4 approved these changes Mar 19, 2024

View reviewed changes

alan-agius4 removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Mar 19, 2024

mlz11 added 2 commits March 19, 2024 10:56

fix(http): include transferCache when cloning HttpRequest

bfb050d

Fixes a bug where HttpRequest.clone() does not include the transferCache property. Fixes angular#54924.

refactor(http): use nullish coalescing operator in HttpRequest.clone()

89e3d25

Refactor how boolean options are handled in HttpRequest.clone() method by using nullish coalescing operator

mlz11 force-pushed the include-transferCache-in-http-request-clone-method branch from 8a90bb8 to 89e3d25 Compare March 19, 2024 10:56

kbrilla reviewed Mar 19, 2024

View reviewed changes

alan-agius4 added the action: review The PR is still awaiting reviews from at least one requested reviewer label Mar 19, 2024

AndrewKushnir requested a review from alan-agius4 March 19, 2024 16:11

mlz11 changed the title ~~fix(common): include transferCache when cloning HttpRequest~~ fix(hhtp): include transferCache when cloning HttpRequest Mar 19, 2024

mlz11 changed the title ~~fix(hhtp): include transferCache when cloning HttpRequest~~ fix(http): include transferCache when cloning HttpRequest Mar 19, 2024

alan-agius4 removed the request for review from dylhunn March 19, 2024 17:26

alan-agius4 requested review from AndrewKushnir and removed request for alan-agius4 March 19, 2024 17:26

AndrewKushnir approved these changes Mar 19, 2024

View reviewed changes

alan-agius4 approved these changes Mar 19, 2024

View reviewed changes

pullapprove bot requested review from alxhub, dylhunn and pkozlowski-opensource March 19, 2024 17:33

AndrewKushnir removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Mar 19, 2024

alan-agius4 added the action: merge The PR is ready for merge by the caretaker label Mar 19, 2024

dylhunn approved these changes Mar 19, 2024

View reviewed changes

pullapprove bot requested a review from jessicajaniuk March 19, 2024 22:03

jessicajaniuk approved these changes Mar 19, 2024

View reviewed changes

alan-agius4 removed request for alxhub and pkozlowski-opensource March 20, 2024 06:16

atscott approved these changes Mar 20, 2024

View reviewed changes

dylhunn closed this in cf73983 Mar 22, 2024

dylhunn pushed a commit that referenced this pull request Mar 22, 2024

refactor(http): use nullish coalescing operator in HttpRequest.clone() (

b80434a

#54939) Refactor how boolean options are handled in HttpRequest.clone() method by using nullish coalescing operator PR Close #54939

dylhunn pushed a commit that referenced this pull request Mar 22, 2024

fix(http): include transferCache when cloning HttpRequest (#54939)

cb433af

Fixes a bug where HttpRequest.clone() does not include the transferCache property. Fixes #54924. PR Close #54939

dylhunn pushed a commit that referenced this pull request Mar 22, 2024

refactor(http): use nullish coalescing operator in HttpRequest.clone() (

1f7e81a

#54939) Refactor how boolean options are handled in HttpRequest.clone() method by using nullish coalescing operator PR Close #54939

angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(http): include transferCache when cloning HttpRequest #54939

fix(http): include transferCache when cloning HttpRequest #54939

mlz11 commented Mar 19, 2024 •

edited

alan-agius4 left a comment

mlz11 commented Mar 19, 2024 •

edited

alan-agius4 left a comment

alan-agius4 commented Mar 19, 2024

mlz11 commented Mar 19, 2024 •

edited

kbrilla Mar 19, 2024

alan-agius4 left a comment

dylhunn left a comment

jessicajaniuk left a comment

atscott left a comment

dylhunn commented Mar 22, 2024

angular-automatic-lock-bot bot commented Apr 22, 2024

fix(http): include transferCache when cloning HttpRequest #54939

fix(http): include transferCache when cloning HttpRequest #54939

Conversation

mlz11 commented Mar 19, 2024 • edited

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

alan-agius4 left a comment

Choose a reason for hiding this comment

mlz11 commented Mar 19, 2024 • edited

alan-agius4 left a comment

Choose a reason for hiding this comment

alan-agius4 commented Mar 19, 2024

mlz11 commented Mar 19, 2024 • edited

kbrilla Mar 19, 2024

Choose a reason for hiding this comment

alan-agius4 left a comment

Choose a reason for hiding this comment

dylhunn left a comment

Choose a reason for hiding this comment

jessicajaniuk left a comment

Choose a reason for hiding this comment

atscott left a comment

Choose a reason for hiding this comment

dylhunn commented Mar 22, 2024

angular-automatic-lock-bot bot commented Apr 22, 2024

mlz11 commented Mar 19, 2024 •

edited

mlz11 commented Mar 19, 2024 •

edited

mlz11 commented Mar 19, 2024 •

edited