fix(common): skip transfer cache on client by Jefftopia · Pull Request #55012 · angular/angular

Jefftopia · 2024-03-23T16:34:46Z

transfer cache interceptor should not run again on the client as it is intended for server to client handoff

PR Type

What kind of change does this PR introduce?

[ X] 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?

In rare instances, the transfer cache interceptor may execute on the client. While this does not appear to introduce problems, it is an unnecessary step.

Issue Number: #54444

What is the new behavior?

The transfer cache interceptor no longer performs caching behavior in the 'broswer' platform.

Does this PR introduce a breaking change?

Yes
No

AndrewKushnir

@Jefftopia thanks for creating the PR 👍

I've left a comment and also added @alan-agius4 as a reviewer to take a look at the change.

packages/common/http/src/transfer_cache.ts

AndrewKushnir · 2024-03-23T20:45:15Z

@Jefftopia one additional comment: I'd propose to change the commit message (via git commit --amend) from feat(common) -> fix(common) (or perf(common)), this change looks more like a fix or a perf improvement vs a new feature.

sonukapoor · 2024-03-23T21:44:06Z

Hey @Jefftopia - Welcome :)

packages/common/http/src/transfer_cache.ts

sonukapoor · 2024-03-24T00:32:14Z

@Jefftopia Its usually better to push additional commits up (instead of modifying the existing commit), so that its easier to see the changes from commit to commit.

Jefftopia · 2024-03-24T00:32:56Z

@sonukapoor oh hey 👋

Missed your comment before but I won't squash everything going forward, thanks.

sonukapoor · 2024-03-24T00:38:26Z

Thanks @Jefftopia Changes LGTM. Lets's wait for Alans input.

packages/common/http/test/transfer_cache_spec.ts

alan-agius4

On the browser only adding to setting to the cache should be skipped

angular/packages/common/http/src/transfer_cache.ts

Line 173 in f02f848

transferState.set<TransferHttpResponse>(storeKey, {

As the current change will cause the cache not to be used at all which is undesired.

packages/common/http/test/transfer_cache_spec.ts

packages/common/http/src/transfer_cache.ts

alan-agius4

Can you please amend the last 2 commits to use fixup!

IE: fixup! fix(common): skip transfer cache on client

Jefftopia · 2024-03-25T21:48:03Z

done

AndrewKushnir · 2024-03-25T22:36:38Z

@Jefftopia thanks for updating the PR. It looks like there is still a second commit (with the "Merge branch 'main' of ..." commit message). Could you please rebase and update the branch to have just 1 commit with all changes? Thank you.

Jefftopia · 2024-03-26T01:15:18Z

@Jefftopia thanks for updating the PR. It looks like there is still a second commit (with the "Merge branch 'main' of ..." commit message). Could you please rebase and update the branch to have just 1 commit with all changes? Thank you.

@AndrewKushnir
It's a merge commit. You can't rebase merge commits. They can be squashed onto main and I believe Github provides you the option to do this. Are there guidelines around merge commits? I didn't see them in the developer docs.

Edit: I went ahead and reset then rebased. Oof.

sonukapoor · 2024-03-26T01:38:48Z

@Jefftopia You could try to drop that commit and rebase again from main.

Jefftopia · 2024-03-26T02:21:24Z

@Jefftopia You could try to drop that commit and rebase again from main.

Done.

sonukapoor · 2024-03-26T02:29:36Z

Nice 👍

I usually use this git command to add a fixup commit: git commit --fixup <COMMIT_ID_OF_FIRST_COMMIT>

sonukapoor

🎉

Jefftopia · 2024-03-26T12:09:43Z

Nice 👍

I usually use this git command to add a fixup commit: git commit --fixup <COMMIT_ID_OF_FIRST_COMMIT>

@sonukapoor Yes, I used fixup for the commits addressing but you cannot fixup a merge commit ;-) Lesson learned for me -- you can only rebase on another branch to avoid merge commits.

Jefftopia · 2024-03-26T12:42:30Z

@alan-agius4 Thanks for your patience here. After "squashing" my merge commit, hydration specs failed. The PLATFORM_ID token was resolved as an InjectionToken object rather than a string. My latest push uses EnvironmentInjector again which contains the expected value in the hydration tests.

I understand the interceptor has its own injection context but would be curious to know why that is to be preferred over environment injector for PLATFORM_ID in particular @AndrewKushnir

packages/common/http/src/transfer_cache.ts

transfer cache interceptor should not run again on the client as it is intended for server to client handoff

JeanMeche · 2024-03-27T02:13:31Z

packages/common/http/test/transfer_cache_spec.ts

+          TestBed.configureTestingModule({
+            declarations: [SomeComponent],
+            providers: [
+              {provide: PLATFORM_ID, useValue: 'browser'},


Nit: Ideally we should be using PLATFORM_BROWSER_ID instead of the browser string. Same goes for the server string in the cases below.

Looks like this has not been addressed.

Jefftopia · 2024-04-04T01:54:42Z

Bump

alan-agius4

LGTM

alan-agius4

LGTM, although technically the commit scope should be http and not common.

alan-agius4 · 2024-04-09T16:00:12Z

packages/common/http/src/transfer_cache.ts

  }

-  // Request not found in cache. Make the request and cache it.
+  const isServer = isPlatformServer(inject(PLATFORM_ID));


NIT: We could use isPlatformBrowser here to avoid the additional symbol. (Couple of less bytes)

atscott · 2024-04-09T21:45:44Z

This PR was merged into the repository by commit 11705f5.

transfer cache interceptor should not run again on the client as it is intended for server to client handoff PR Close #55012

transfer cache interceptor should not run again on the client as it is intended for server to client handoff PR Close angular#55012

angular-automatic-lock-bot · 2024-05-11T00:09:18Z

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 23, 2024 16:34

angular-robot bot added the detected: feature PR contains a feature commit label Mar 23, 2024

AndrewKushnir reviewed Mar 23, 2024

View reviewed changes

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

AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer area: common/http Issues related to HTTP and HTTP Client target: patch This PR is targeted for the next patch release server: http cache labels Mar 23, 2024

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

AndrewKushnir requested review from alan-agius4 and removed request for dylhunn March 23, 2024 20:43

Jefftopia force-pushed the feat/skip-transfer-cache-browser branch from 6f8b48a to 6dad001 Compare March 23, 2024 23:57

angular-robot bot removed the detected: feature PR contains a feature commit label Mar 23, 2024

Jefftopia force-pushed the feat/skip-transfer-cache-browser branch from 6dad001 to f62f423 Compare March 24, 2024 00:00

sonukapoor reviewed Mar 24, 2024

View reviewed changes

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

Jefftopia force-pushed the feat/skip-transfer-cache-browser branch from f62f423 to f02f848 Compare March 24, 2024 00:31

JeanMeche reviewed Mar 24, 2024

View reviewed changes

packages/common/http/test/transfer_cache_spec.ts Outdated Show resolved Hide resolved

alan-agius4 requested changes Mar 24, 2024

View reviewed changes

alan-agius4 reviewed Mar 24, 2024

View reviewed changes

packages/common/http/test/transfer_cache_spec.ts Outdated Show resolved Hide resolved

JeanMeche reviewed Mar 25, 2024

View reviewed changes

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

JeanMeche reviewed Mar 25, 2024

View reviewed changes

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

alan-agius4 requested changes Mar 25, 2024

View reviewed changes

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 25, 2024

Jefftopia force-pushed the feat/skip-transfer-cache-browser branch from 42df598 to a8d132a Compare March 25, 2024 21:47

AndrewKushnir changed the title ~~feat(common): skip transfer cache on client #54444~~ fix(common): skip transfer cache on client Mar 25, 2024

Jefftopia force-pushed the feat/skip-transfer-cache-browser branch from 0dbeb44 to 72a23c1 Compare March 26, 2024 02:16

sonukapoor approved these changes Mar 26, 2024

View reviewed changes

JeanMeche reviewed Mar 26, 2024

View reviewed changes

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

fix(common): skip transfer cache on client

6285c78

transfer cache interceptor should not run again on the client as it is intended for server to client handoff

Jefftopia force-pushed the feat/skip-transfer-cache-browser branch from 222743e to 6285c78 Compare March 27, 2024 01:23

AndrewKushnir requested a review from alan-agius4 March 27, 2024 01:51

AndrewKushnir 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 27, 2024

JeanMeche reviewed Mar 27, 2024

View reviewed changes

alan-agius4 approved these changes Apr 9, 2024

View reviewed changes

alan-agius4 reviewed Apr 9, 2024

View reviewed changes

alan-agius4 added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 9, 2024

atscott pushed a commit that referenced this pull request Apr 9, 2024

fix(common): skip transfer cache on client (#55012)

766548c

transfer cache interceptor should not run again on the client as it is intended for server to client handoff PR Close #55012

atscott closed this in 11705f5 Apr 9, 2024

jsaguet mentioned this pull request Apr 10, 2024

feat(http): allow caching requests with different origins between ser… #55274

Closed

6 tasks

iteriani pushed a commit to iteriani/angular that referenced this pull request Apr 11, 2024

fix(common): skip transfer cache on client (angular#55012)

a8a76c2

transfer cache interceptor should not run again on the client as it is intended for server to client handoff PR Close angular#55012

alan-agius4 mentioned this pull request Apr 24, 2024

Shouldn't TransferState.set be called only on server side? #54444

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators May 11, 2024

Conversation

Jefftopia commented Mar 23, 2024 • edited by alan-agius4 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AndrewKushnir commented Mar 23, 2024

Uh oh!

sonukapoor commented Mar 23, 2024

Uh oh!

Uh oh!

sonukapoor commented Mar 24, 2024

Uh oh!

Jefftopia commented Mar 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonukapoor commented Mar 24, 2024

Uh oh!

Uh oh!

alan-agius4 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alan-agius4 left a comment

Choose a reason for hiding this comment

Uh oh!

Jefftopia commented Mar 25, 2024

Uh oh!

AndrewKushnir commented Mar 25, 2024

Uh oh!

Jefftopia commented Mar 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonukapoor commented Mar 26, 2024

Uh oh!

Jefftopia commented Mar 26, 2024

Uh oh!

sonukapoor commented Mar 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonukapoor left a comment

Choose a reason for hiding this comment

Uh oh!

Jefftopia commented Mar 26, 2024

Uh oh!

Jefftopia commented Mar 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

JeanMeche Mar 27, 2024

Choose a reason for hiding this comment

Uh oh!

alan-agius4 Apr 9, 2024

Choose a reason for hiding this comment

Uh oh!

Jefftopia commented Apr 4, 2024

Uh oh!

alan-agius4 left a comment

Choose a reason for hiding this comment

Uh oh!

alan-agius4 left a comment

Choose a reason for hiding this comment

Uh oh!

alan-agius4 Apr 9, 2024

Choose a reason for hiding this comment

Uh oh!

atscott commented Apr 9, 2024

Uh oh!

angular-automatic-lock-bot bot commented May 11, 2024

Uh oh!

Reviewers

Jefftopia commented Mar 23, 2024 •

edited by alan-agius4

Loading

Jefftopia commented Mar 24, 2024 •

edited

Loading

alan-agius4 left a comment •

edited

Loading

Jefftopia commented Mar 26, 2024 •

edited

Loading

sonukapoor commented Mar 26, 2024 •

edited

Loading

Jefftopia commented Mar 26, 2024 •

edited

Loading