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: prevent re-render on issue fetching and improve PWA #136

Merged
merged 13 commits into from
Oct 29, 2024

Conversation

zugdev
Copy link
Contributor

@zugdev zugdev commented Oct 23, 2024

#131 introduced issue caching to improve performance on poor networks. Now we render issues from cache and fetch in parallel. If cached issues aren't up-to-date we cache fetched issues and render them. However, this leads to a replay in the appearing issues animation. This PR introduces an option to skip this animation, handling both the case where a stale issue is removed and a new issue is introduced. Here is a demo:

fix-rerender-demo.webm

@zugdev zugdev requested a review from 0x4007 as a code owner October 23, 2024 13:54
@ubiquity-os-deployer
Copy link

ubiquity-os-deployer bot commented Oct 23, 2024

@zugdev zugdev changed the title fix: prevent re-render on issue fetching fix: prevent re-render on issue fetching and improve PWA Oct 23, 2024
@zugdev
Copy link
Contributor Author

zugdev commented Oct 23, 2024

I've also changed the strategy in the PWA. We should default to Network and if offline load from cache. Defaulting to cache is horrible DX because you constantly need to clear cache in order to see update in code. This needs review and merge ASAP @0x4007

My vercel deployment: https://work-ubq-fi-git-fix-rerender-zugs-projects-e51b52e0.vercel.app

@0x4007
Copy link
Member

0x4007 commented Oct 24, 2024

Vercel is private but requested access. Why use vercel and not our continuous deploy?

@zugdev
Copy link
Contributor Author

zugdev commented Oct 25, 2024

Vercel is private but requested access. Why use vercel and not our continuous deploy?

No difference tbh, I just have it setup for CD in my fork and since the CD here was pending I attached.

await taskManager.syncTasks();
void displayGitHubIssues();
if (cachedIssues) {
void displayGitHubIssues(undefined, undefined, true); // if there were cached issues skip animation
Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense to change the function signature so that you don't have to pass in two undefined?

Alternatively, if it makes sense, pass in destructured parameters (an object)

@0x4007
Copy link
Member

0x4007 commented Oct 28, 2024

When I load the first time there is no animation.

@zugdev
Copy link
Contributor Author

zugdev commented Oct 29, 2024

@0x4007 this should be better now, the problem was PWA would serve from cache always and we would need to manually update cache's name to ensure users get distributed updates. I've designed it now so that whenever you open the page you load the newest version into cache on the background, so next time users open the page/app they'll have the newest version.

@zugdev
Copy link
Contributor Author

zugdev commented Oct 29, 2024

Still keeping cache first approach since it's much better in performance.

@zugdev zugdev requested a review from 0x4007 October 29, 2024 00:29
return cachedResponse;
}
return fetch(event.request)
const fetchPromise = fetch(event.request)
.then((networkResponse) => {
Copy link
Member

Choose a reason for hiding this comment

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

Can you do async await syntax?

@0x4007 0x4007 merged commit 3e0270d into ubiquity:development Oct 29, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants