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.

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

fix: cache fix for virtuoso (#4201) #4219

Closed
wants to merge 2 commits into from
Closed

fix: cache fix for virtuoso (#4201) #4219

wants to merge 2 commits into from

Conversation

RutamBhagat
Copy link

@RutamBhagat RutamBhagat commented Dec 12, 2023

What does this PR do?

Fixes issue #4201 by caching user profiles

Related issues

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Enhancement (non-breaking small changes to existing functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Explanation of the changes

/claim #4201

simplescreenrecorder-2023-12-12_23.34.08.mp4

Copy link

vercel bot commented Dec 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
web ✅ Ready (Inspect) Visit Preview Dec 14, 2023 10:36am

@rishi-raj-jain
Copy link

@RutamBhagat I didn't know that this can be done. A learning!

@bigint
Copy link
Member

bigint commented Dec 13, 2023

It is not intended to see cache requests in network too, if you see twitter they do virtual scroll nicely 🤔

@bigint
Copy link
Member

bigint commented Dec 13, 2023

Screen.Recording.2023-12-13.at.11.27.15.AM.mov

@bigint
Copy link
Member

bigint commented Dec 13, 2023

Screen.Recording.2023-12-13.at.11.30.31.AM.mov

they unmount too

@rishi-raj-jain
Copy link

Damn 🤯

@bigint
Copy link
Member

bigint commented Dec 13, 2023

Twitter is always a engineering marvel lol 😅

@rishi-raj-jain
Copy link

Screen.Recording.2023-12-13.at.11.30.31.AM.mov
they unmount too

Can you look into the tags they've used? are we sure they are loading images and not buffers?

@rishi-raj-jain
Copy link

Oh no they are also refetching

@rishi-raj-jain
Copy link

here goes 👇🏻

Screen.Recording.2023-12-13.at.12.41.01.PM.mov

@RutamBhagat
Copy link
Author

RutamBhagat commented Dec 13, 2023

It is not intended to see cache requests in network too, if you see twitter they do virtual scroll nicely 🤔

Just as Rishi mentioned Twitter cache requests are also visible in the network

simplescreenrecorder-2023-12-13_14.40.17.mp4

@bigint
Copy link
Member

bigint commented Dec 14, 2023

@rishi-raj-jain @RutamBhagat this happens only if you disable cache on dev tools! if you disable it will not be called again!

@bigint
Copy link
Member

bigint commented Dec 14, 2023

If its something related to disk cache we don't want to do any code changes, it can be easily done on network layer!

@bigint
Copy link
Member

bigint commented Dec 14, 2023

Will react memo fix it?

@RutamBhagat
Copy link
Author

@rishi-raj-jain @RutamBhagat this happens only if you disable cache on dev tools! if you disable it will not be called again!

I am sorry, I am a bit confused, when I disable caching on twitter it still tries to refetch the images

simplescreenrecorder-2023-12-14_17.23.58.mp4

@RutamBhagat
Copy link
Author

Will react memo fix it?

No it still shows that the request is cached in the networks tab but I feel like React.memo is a cleaner implementation

hey.list.mp4

@bigint
Copy link
Member

bigint commented Dec 14, 2023

Request is cached because we added cache headers on firewall!

@bigint
Copy link
Member

bigint commented Dec 14, 2023

@rishi-raj-jain @RutamBhagat this happens only if you disable cache on dev tools! if you disable it will not be called again!

I am sorry, I am a bit confused, when I disable caching on twitter it still tries to refetch the images

simplescreenrecorder-2023-12-14_17.23.58.mp4

you need to disable aka uncheck the option

@bigint
Copy link
Member

bigint commented Dec 15, 2023

Closing this, we can move discussion to issue!

@bigint bigint closed this Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants