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

feat: Add swr cache #437

Merged
merged 3 commits into from
Sep 30, 2022
Merged

feat: Add swr cache #437

merged 3 commits into from
Sep 30, 2022

Conversation

bdougie
Copy link
Member

@bdougie bdougie commented Sep 26, 2022

What type of PR is this? (check all applicable)

  • πŸ• Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation Update
  • 🎨 Style
  • πŸ§‘β€πŸ’» Code Refactor
  • πŸ”₯ Performance Improvements
  • βœ… Test
  • πŸ€– Build
  • πŸ” CI
  • πŸ“¦ Chore (Release)
  • ⏩ Revert

Description

Fetching content from the API could be cached and this PR applies that patter to all hooks

Details

  • useRepositoriesList
  • useRepositoryCommits
  • useRepositoriyPRs
  • useContributorData
  • useContributorListuseNav

Related Tickets & Documents

fix #308

Mobile & Desktop Screenshots/Recordings

Added tests?

  • πŸ‘ yes
  • πŸ™… no, because they aren't needed
  • πŸ™‹ no, because I need help

Added to documentation?

  • πŸ“œ README.md
  • πŸ““ docs.opensauced.pizza
  • πŸ• dev.to/opensauced
  • πŸ“• storybook
  • πŸ™… no documentation needed

[optional] Are there any post-deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

@netlify
Copy link

netlify bot commented Sep 26, 2022

βœ… Deploy Preview for oss-insights ready!

Name Link
πŸ”¨ Latest commit fe3f4e7
πŸ” Latest deploy log https://app.netlify.com/sites/oss-insights/deploys/6334c72b3ba3ad00080034d0
😎 Deploy Preview https://deploy-preview-437--oss-insights.netlify.app
πŸ“± Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Sep 26, 2022

βœ… Deploy Preview for design-insights ready!

Name Link
πŸ”¨ Latest commit fe3f4e7
πŸ” Latest deploy log https://app.netlify.com/sites/design-insights/deploys/6334c72bca52e30009551bbf
😎 Deploy Preview https://deploy-preview-437--design-insights.netlify.app
πŸ“± Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@bdougie bdougie marked this pull request as ready for review September 28, 2022 22:14
@bdougie
Copy link
Member Author

bdougie commented Sep 28, 2022

After some test yesterday, it seems like the caching works with this change. SWR works best with Next.js and no extra configuration should be needed. Marked this for review so we can test the caching on a production URL

@0-vortex
Copy link
Contributor

After some test yesterday, it seems like the caching works with this change. SWR works best with Next.js and not extra configuration should be needed. Marked this for review so we can test the caching on a production URL

Glad it works out of the box, actually looked at why this is after your last stream and it is preconfigured for nextjs rendering - for this PR there is the question of whether we can use refreshInterval globally or we need to provide some custom value to 1-2 hooks. I don't think any of the other options are really needed for us.

We are not really mutating anything right now anyway! πŸ•

@bdougie
Copy link
Member Author

bdougie commented Sep 29, 2022

for this PR there is the question of whether we can use refreshInterval globally or if we need to provide some custom value to 1-2 hooks. I don't think any of the other options are really needed for us.

Curious about this too. If we have the ETL updating data on a cron for X amount of time, it might be worth following slightly longer than that interval would be.

@0-vortex
Copy link
Contributor

for this PR there is the question of whether we can use refreshInterval globally or if we need to provide some custom value to 1-2 hooks. I don't think any of the other options are really needed for us.

Curious about this too. If we have the ETL updating data on a cron for X amount of time, it might be worth following slightly longer than that interval would be.

Was thinking of "days" to be honest, etl is minutes right now πŸ˜…

@bdougie
Copy link
Member Author

bdougie commented Sep 30, 2022

So after some testing on the deploy preview vs production url. I have come to the conclusion that this is all working as expected and this PR is possibly not adding nay value.

deploy preview

Screen Shot 2022-09-29 at 6 52 14 PM

production insights.opensauced.pizza

Screen Shot 2022-09-29 at 6 52 19 PM

My assumption is the addition of the localstorage is not a performance win unless we are tapping the cache directly, but what I am seeing around is that is not recommended.

So my question is:

Should we go with this and replace localStorage in the event we add a state management tool in the future or stick with the defaults, and close this PR and issue for now?

@0-vortex
Copy link
Contributor

So after some testing on the deploy preview vs production url. I have come to the conclusion that this is all working as expected and this PR is possibly not adding nay value.

deploy preview

Screen Shot 2022-09-29 at 6 52 14 PM

production insights.opensauced.pizza

Screen Shot 2022-09-29 at 6 52 19 PM

My assumption is the addition of the localstorage is not a performance win unless we are tapping the cache directly, but what I am seeing around is that is not recommended.

So my question is:

Should we go with this and replace localStorage in the event we add a state management tool in the future or stick with the defaults, and close this PR and issue for now?

Partially correct, the default cache is a memory buffer that is pretty big on our turbo charged machines. The local storage would indeed add value in the long term as we would cache stuff for low end machines as well and for a longer period of time.

I would say that even without that, this PR would be a win by simply enforcing good cache and revalidate timeouts to force swr to do the updates instead of us making an extra wrapper to write to the localstorage (no gains, just code)

πŸ• πŸ• πŸ•

@brandonroberts
Copy link
Contributor

I think we will end up having a combination, where we have some centralized shared state that gets synced to local storage similar to what we have now.

I think the PR helps either way also

@bdougie bdougie merged commit 6b2b1cb into beta Sep 30, 2022
@bdougie bdougie deleted the 308-swr-cache branch September 30, 2022 05:44
@bdougie
Copy link
Member Author

bdougie commented Sep 30, 2022

ok, based on that. going to merge and move forward.

github-actions bot pushed a commit that referenced this pull request Sep 30, 2022
## [1.8.0-beta.5](v1.8.0-beta.4...v1.8.0-beta.5) (2022-09-30)

### πŸ• Features

* Add swr cache ([#437](#437)) ([6b2b1cb](6b2b1cb)), closes [#308](#308)
@github-actions
Copy link
Contributor

πŸŽ‰ This PR is included in version 1.8.0-beta.5 πŸŽ‰

The release is available on GitHub release

Your semantic-release bot πŸ“¦πŸš€

github-actions bot pushed a commit that referenced this pull request Oct 3, 2022
## [1.9.0](v1.8.0...v1.9.0) (2022-10-03)

### πŸ› Bug Fixes

* add truncate to contributors repositories list tab ([#472](#472)) ([b7ec96e](b7ec96e)), closes [#460](#460)
* fix merged PRs that looks like closed  ([#475](#475)) ([1dd820a](1dd820a)), closes [#462](#462)
* issue template πŸ‘‹πŸΎ ([31b7a7e](31b7a7e))
* moved page head and footer to repo page to avoid reloading the entire screen ([#473](#473)) ([bf790e8](bf790e8)), closes [#449](#449)
* replace churn with overview ([#470](#470)) ([fc0fe3f](fc0fe3f)), closes [#469](#469)
* SEO descriptions ([980e01d](980e01d))

### πŸ• Features

* add filters to hooks for repos and contributions ([#458](#458)) ([05e6bbe](05e6bbe)), closes [#457](#457)
* add page title and info to repo and contributors page ([#459](#459)) ([c0000e4](c0000e4)), closes [#446](#446)
* add pagination to contributors page ([#454](#454)) ([37f4da8](37f4da8)), closes [#403](#403)
* Add swr cache ([#437](#437)) ([6b2b1cb](6b2b1cb)), closes [#308](#308)
* connect scatter chart to contributors data ([#466](#466)) ([be78024](be78024)), closes [#465](#465)
* hide show/hide buttons temporarily ([#474](#474)) ([2ce5b3c](2ce5b3c)), closes [#439](#439)
* Leverage cloudinary for round images ([#467](#467)) ([7dc64d8](7dc64d8)), closes [#373](#373)
* update filters for repos and contributors ([#464](#464)) ([e15bbf8](e15bbf8)), closes [#461](#461)
ElpisHelle added a commit to ElpisHelle/next.js-tailwindcss that referenced this pull request Aug 17, 2023
## [1.8.0-beta.5](open-sauced/app@v1.8.0-beta.4...v1.8.0-beta.5) (2022-09-30)

### πŸ• Features

* Add swr cache ([#437](open-sauced/app#437)) ([6b2b1cb](open-sauced/app@6b2b1cb)), closes [#308](open-sauced/app#308)
ElpisHelle added a commit to ElpisHelle/next.js-tailwindcss that referenced this pull request Aug 17, 2023
## [1.9.0](open-sauced/app@v1.8.0...v1.9.0) (2022-10-03)

### πŸ› Bug Fixes

* add truncate to contributors repositories list tab ([#472](open-sauced/app#472)) ([b7ec96e](open-sauced/app@b7ec96e)), closes [#460](open-sauced/app#460)
* fix merged PRs that looks like closed  ([#475](open-sauced/app#475)) ([1dd820a](open-sauced/app@1dd820a)), closes [#462](open-sauced/app#462)
* issue template πŸ‘‹πŸΎ ([31b7a7e](open-sauced/app@31b7a7e))
* moved page head and footer to repo page to avoid reloading the entire screen ([#473](open-sauced/app#473)) ([bf790e8](open-sauced/app@bf790e8)), closes [#449](open-sauced/app#449)
* replace churn with overview ([#470](open-sauced/app#470)) ([fc0fe3f](open-sauced/app@fc0fe3f)), closes [#469](open-sauced/app#469)
* SEO descriptions ([980e01d](open-sauced/app@980e01d))

### πŸ• Features

* add filters to hooks for repos and contributions ([#458](open-sauced/app#458)) ([05e6bbe](open-sauced/app@05e6bbe)), closes [#457](open-sauced/app#457)
* add page title and info to repo and contributors page ([#459](open-sauced/app#459)) ([c0000e4](open-sauced/app@c0000e4)), closes [#446](open-sauced/app#446)
* add pagination to contributors page ([#454](open-sauced/app#454)) ([37f4da8](open-sauced/app@37f4da8)), closes [#403](open-sauced/app#403)
* Add swr cache ([#437](open-sauced/app#437)) ([6b2b1cb](open-sauced/app@6b2b1cb)), closes [#308](open-sauced/app#308)
* connect scatter chart to contributors data ([#466](open-sauced/app#466)) ([be78024](open-sauced/app@be78024)), closes [#465](open-sauced/app#465)
* hide show/hide buttons temporarily ([#474](open-sauced/app#474)) ([2ce5b3c](open-sauced/app@2ce5b3c)), closes [#439](open-sauced/app#439)
* Leverage cloudinary for round images ([#467](open-sauced/app#467)) ([7dc64d8](open-sauced/app@7dc64d8)), closes [#373](open-sauced/app#373)
* update filters for repos and contributors ([#464](open-sauced/app#464)) ([e15bbf8](open-sauced/app@e15bbf8)), closes [#461](open-sauced/app#461)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Front-End] Implement persistent caching layer in the project
3 participants