Skip to content
This repository has been archived by the owner on Aug 18, 2023. It is now read-only.

Update Pages data with SWR #434

Open
wants to merge 390 commits into
base: bezos-main
Choose a base branch
from
Open

Update Pages data with SWR #434

wants to merge 390 commits into from

Conversation

ladytrekker
Copy link
Collaborator

@ladytrekker ladytrekker commented Mar 28, 2023

Description

This PR integrates SWR to the following pages

  • Single Project Page (initial data rendered from NextJS staticProps, update page data every 40 seconds)
  • Profile (initial data rendered from NextJS staticProps, fetch user data again after editing data on Frontend)
  • Portfolio (fetch user data on initial render on Frontend, fetch again after editing data on Frontend)

With this integration, the following caching "bugs" related to NextJS page rendering behaviour are FIXED:

  1. update a listing / edit a profile / create a listing
  2. Go to another page
  3. Go back to Profile / Portfolio
  4. => the updated data is not there anymore (Page is rendered from NextJS cache)

Or

  1. Make a Purchase
  2. Go back to the Project Single Page
  3. => the purchased listing is still there
  4. => The activity data is not updated

All this should be fixed now ✅

Please test this PR heavily 🙏 Thanks!

Related Ticket

Closes https://github.com/Atmosfearful/bezos-frontend/issues/357

Changes

Another tiny change in this PR:

When a project can not be found
=> the page is not rendered no more! Instead the request fails with a 404

Before: https://www.carbonmark.com/projects/not-a-project
After: https://carbonmark-git-ladytrekker-swr-klimadao.vercel.app/projects/not-a-project

Before After
Bildschirm­foto 2023-03-27 um 15 38 51 Bildschirm­foto 2023-03-27 um 15 39 01

Checklist

  • I have run npm run build-all without errors
  • I have formatted JS and TS files with npm run format-all

Copy link
Collaborator

@0xemc 0xemc left a comment

Choose a reason for hiding this comment

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

It looks like logging out does not revert back to the unauthenticated Profile page

image

carbonmark/components/pages/Project/index.tsx Show resolved Hide resolved
Comment on lines 116 to 157
// when carbonmark user data changed
useEffect(() => {
// store first data and return
if (!initialUser.current && carbonmarkUser) {
initialUser.current = carbonmarkUser;
return;
}

const fetchUser = () =>
getUser({
user: props.userAddress,
type: "wallet",
});
// when activity data differs => reset state and update initial user
if (
initialUser.current &&
carbonmarkUser &&
getActivityIsUpdated(initialUser.current, carbonmarkUser)
) {
setInterval(0);
setIsUpdatingUser(false);
initialUser.current = carbonmarkUser;
fetchIntervalTimer.current && clearTimeout(fetchIntervalTimer.current);
}
}, [carbonmarkUser]);

// API is updated when new activity exists
const activityIsAdded = (value: User) => {
const newActivityLength = value.activities.length;
const currentActivityLength = user.activities.length;
return newActivityLength > currentActivityLength;
};
// on unmount
useEffect(() => {
return () => {
fetchIntervalTimer.current && clearTimeout(fetchIntervalTimer.current);
};
}, []);

const updatedUser = await pollUntil({
fn: fetchUser,
validate: activityIsAdded,
ms: 1000,
maxAttempts: 50,
});
const onUpdateUser = async () => {
// increase fetch interval
// the useEffect above will take care of updated carbonmark user data
setInterval(1000);
setIsUpdatingUser(true);

setUser((prev) => ({ ...prev, ...updatedUser }));
} catch (e) {
console.error("LOAD USER ACTIVITY error", e);
// Show error message after 50 seconds
fetchIntervalTimer.current = setTimeout(() => {
setInterval(0);
setIsUpdatingUser(false);
setErrorMessage(
t`Please refresh the page. There was an error updating your data: ${e}.`
t`Please refresh the page. There was an error updating your data.`
);
} finally {
setIsUpdatingUser(false);
}
}, 50000);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could also be removed if we update useFetchUser with the timeout arg

Comment on lines +3 to +7
export const getActivityIsUpdated = (oldUser: User, newUser: User) => {
const formerActivityLength = oldUser?.activities?.length || 0;
const newActivityLength = newUser?.activities?.length || 0;
return newActivityLength > formerActivityLength;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

This probably isn't needed if you're no longer managing two user states

Copy link
Collaborator Author

@ladytrekker ladytrekker Mar 31, 2023

Choose a reason for hiding this comment

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

Thanks for the suggestion!

I was struggling here with SWR because what I want to achieve is this:

Under a certain condition => start fetching until the resource is updated with fresh data

I thought SWR would handle this for me:
Fetch again and again and validate if the data is updated

I can trigger one fetch, yes.
But what if the backend needs longer?
Sometimes even 50 seconds to be updated?

SWR did not tell me how to achieve this. There is this option "compare(a, b)" in the API docs but this did not work.

So what I went for is this:

  • When a user contract transaction was successful => increase the SWR fetch interval for the user backend endpoint
  • compare the former user data with the new incoming data (every XY second)
  • When the new user data contains new activities => backend is done! Decrease SWR fetch interval
  • If the SWR fetch interval takes too long (backend needs forever) => timeout! Decrease the interval and show an error message

SWR, what am I missing?
Any idea on this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah ok I misunderstood your intention.

If I understand correctly, you'd like an increasing fetch interval with multiple errors until the response contains some condition? (number of activities > 0 for example) That makes sense.

SWR uses an exponential back-off algorithm on errors (see error-retry) so the behaviour you're looking for should come out of the box.

We can leverage this with something like:

export class NoUserActivitiesError extends Error {
  public user: User;
  constructor(user: User) {
    super("User has no activities");
    this.name = "NoActivitiesError";
    this.user = user;
  }
}

// Fetches a user from the Carbonmark API
export const useFetchUser = (address?: string, options?: SWRConfiguration) => {
  const { data, ...rest } = useSWR<User, Error>(
    address ? `/api/users/${address}?type=wallet` : null,
    async (url) => {
      const user: User = await fetcher(url);
      if (user.activities.length === 0) {
        throw new NoUserActivitiesError(user);
      }
      return user;
    },
    {
      ...options,
      errorRetryCount: 5,
    }
  );
  const carbonmarkUser = !!data?.handle ? data : null;

  return { carbonmarkUser, ...rest };
};

You can now use the errorRetryCount to determine how many times swr will attempt to refetch within the default timeout time (the time set by the browser by default).

Does this achieve what you're after? It would be preferable to duplicating code and managing multiple states and timeouts etc (for those devs that need to work on this in future)

One thing to consider is also what if the user genuinely has no activities? I think this may be an issue with the API where by it should return nothing until it has resolved activities (rather than return an empty array)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, I will give this a try.

Just for completion:
Checking for simply > 0 is not enough.
The condition is met when the new user's activity length is higher than the former one.
So we will always need the former data for comparison.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry so you have a pending fetch for increases to the number of activities? Is there a reason why invalidating the swr key (triggering a refetch) on any action would not achieve this sync that you're after? The query should also invalidate on tab switch etc so that should be covered.

carbonmark/hooks/useFetchUser.ts Show resolved Hide resolved
@ladytrekker
Copy link
Collaborator Author

ladytrekker commented Mar 31, 2023

I think if you are on your profile page logged in (https://www.carbonmark.com/users/jabby) and I click the LOG OUT button, I should be redirected to https://www.carbonmark.com/users/login

(note: this is the behaviour on the Portfolio page)

@jabby09 @0xemc

This discussion is unrelated to this PR and just raised again here which is not the correct place

Please continue the discussion in this ticket here:
https://github.com/Atmosfearful/bezos-frontend/issues/458

@0xemc
Copy link
Collaborator

0xemc commented Apr 4, 2023

It looks like logging out does not revert back to the unauthenticated Profile page
image

This is intended behavior because the profile page is for logged-in users only:

  • Profile Page not-logged-in => Login Screen and main menu highlights "Profile"
  • Profile Page logged-in => Carbonmark user data and main menu highlights "Profile"
  • Log-Out while on Profile => Show User page and main menu highlights "Marketplace"

See this discussion here too

Oh I see, so the issue is that looking at my own profile page looks very similar to looking at the user page for my account? And logging out while on my profile will take me to my user page (users/0xemc)?

I can see this causing some confusion with users @jabby09 perhaps logging out should take the user to /users/login what do you think?

Anyway @ladytrekker if that's what's intended then fair enough I'll close this once jabby replies

Copy link
Collaborator

@Atmosfearful Atmosfearful left a comment

Choose a reason for hiding this comment

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

I have an idea that will be a bit more reuseable, and hopefully less complex as it won't need useEffect or refs.

I believe this can be done by awaiting an async function that contains all the retry logic.

Something like this?

const { mutate, data: oldUser } = useSWR("/api/user/ladytrekker");

const handleMutateUser = async () => {
  // start loading spinners
  setPending(true);

  // retry until...
  const newUser = await retryFetchUser(
    (newUser) => newUser.activities.length > oldUser.activities.length
  );

  setPending(false);
  // I'm passing in data here to simulate that we got a fresh record from API
  return newUser;
};

const onUpdateUser = async () => {
  // optional: we can create a synthetic activity and not show the loading spinners
  const optimisticData = { ...oldUser, activities: newActivities };

  mutate(handleMutateUser, {
    revalidate: false,
    populateCache: true,
    optimisticData, // optional
  });
};

I created a demo here that shows the persistent data, including mutating data on screens that haven't mounted yet: https://sxddxw-3000.csb.app/

full demo code https://codesandbox.io/p/sandbox/sweet-cookies-sxddxw?file=%2Fpages%2Findex.tsx&selection=%5B%7B%22endColumn%22%3A8%2C%22endLineNumber%22%3A94%2C%22startColumn%22%3A8%2C%22startLineNumber%22%3A94%7D%5D

import { fetcher } from "lib/fetcher";
import type { SWRConfiguration } from "swr";
import useSWR from "swr";
import { Project } from "../lib/types/carbonmark";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
import { Project } from "../lib/types/carbonmark";
import { Project } from "lib/types/carbonmark";

import { fetcher } from "lib/fetcher";
import type { SWRConfiguration } from "swr";
import useSWR from "swr";
import { User } from "../lib/types/carbonmark";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
import { User } from "../lib/types/carbonmark";
import { User } from "lib/types/carbonmark";

@@ -14,15 +14,16 @@ interface Params extends ParsedUrlQuery {
user: string;
}

interface PageProps {
export interface UserPageStaticProps {
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it a circular dependency when this is used to define the Page component, but Page component is imported here? Maybe best to export from the page component...

carbonmark/hooks/useFetchUser.ts Show resolved Hide resolved
@jabby09
Copy link
Member

jabby09 commented Apr 4, 2023

Oh I see, so the issue is that looking at my own profile page looks very similar to looking at the user page for my account? And logging out while on my profile will take me to my user page (users/0xemc)?

I can see this causing some confusion with users @jabby09 perhaps logging out should take the user to /users/login what do you think?

Anyway @ladytrekker if that's what's intended then fair enough I'll close this once jabby replies

@0xemc @ladytrekker yes I agree it is confusing, I raised this during dev but it was low priority and we had bigger UX issues to solve :)

I think if you are on your profile page logged in (https://www.carbonmark.com/users/jabby) and I click the LOG OUT button, I should be redirected to https://www.carbonmark.com/users/login

(note: this is the behaviour on the Portfolio page)

@ShimonD-KlimaDAO
Copy link

This flow doesn't seem to work well when editing a listing:

  1. update a listing / edit a profile / create a listing
  2. Go to another page
  3. Go back to Profile / Portfolio
  4. => the updated data is not there anymore (Page is rendered from NextJS cache)

"edit 3" disappears when refreshing (one time), and also when switching pages and coming back to Profile.
Screencast from 04-05-2023 03:00:24 PM.webm

I haven't tested other flows yet. Per @Atmosfearful , the firebase env variable hasn't been swapped for staging to point to the mainnet one. I just want to mention this in case it's relevant to the problem, since I had to create a new profile.

@0xemc 0xemc self-requested a review April 6, 2023 06:09
0xemc
0xemc previously approved these changes Apr 6, 2023
@0xemc 0xemc dismissed their stale review April 6, 2023 06:09

Outstanding conversations

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants