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

Ga topic status with localstorage #1604

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

gabriele-ct
Copy link
Contributor

@gabriele-ct gabriele-ct commented Apr 5, 2023

Attempt to use localstorage cache for swr.

I invested some time on it and still has glitches:

  • if a user logs out with, say, 1 quiz passed, then logs in again and passes the 2nd quiz, when navigates from one page to another, keeps getting the old 1 quiz passed response.
  • little flash when navigating to a SSR page (as it doesn't have localstorage knowledge)

@changeset-bot
Copy link

changeset-bot bot commented Apr 5, 2023

🦋 Changeset detected

Latest commit: 1902036

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@commercetools-docs/gatsby-theme-learning Minor
@commercetools-docs/gatsby-theme-docs Minor
@commercetools-website/docs-smoke-test Minor
@commercetools-website/documentation Minor
@commercetools-website/api-docs-smoke-test Patch
@commercetools-website/site-template Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Apr 5, 2023

@gabriele-ct gabriele-ct changed the base branch from main to ga-topic-status April 5, 2023 11:13
isGlobalNotificationVisible={Boolean(props.globalNotification)}
>
{isClientSide ? (
<SWRConfig value={{ provider: localStorageProvider }}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

First doubt. If we want to have the course/topic status available in the sidebar and (potentially in the future) in the actual content page, the only place common between these 2 layouts is the application layout.
Adding cache provider here means that all the usages of swr in the app will be cached in local cache. This might be what we actually want, or might be creating side effects.

Copy link
Collaborator

Choose a reason for hiding this comment

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

#1 I wonder if something like the SWRConfig would not be better placed in gatsby-browser's "wrapXXXElement" hooks. That implicitly makes them client side only and also makes sure they are in all layouts.

#2 having one cache provider for any SWR even across microsites felt weird to me too in the beginning but I concluded that it's the whole point of it. The cache keys are the URLs you use to fetch from SWR and the content is the API response. And we want that information to be available at least in stale state for other pages / views / microsites. So it must be one fixed key like "app-cache" and also at top level for the domain. We could use a more specific localstorage key maybe that is more explanatory.

const map = new Map(JSON.parse(localStorage.getItem('app-cache') || '[]'));

const handleBeforUnload = (event) => {
const appCache = JSON.stringify(Array.from(map.entries()));
Copy link
Contributor Author

@gabriele-ct gabriele-ct Apr 5, 2023

Choose a reason for hiding this comment

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

This handler is rather blind approach as it will simply dump swr state to the cache each time the page is refreshed. But we refresh the page also each time we login and logout.
This is the reason of the 2 custom events that basically disable writing the cache upon login and logout.

Copy link
Collaborator

@nkuehn nkuehn left a comment

Choose a reason for hiding this comment

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

The behavior is nice :-)

Still a slight delay but that is the general hydration of the static page after reloading (it just becomes more visible but is not new, the top meny does not work too in the first second).

isGlobalNotificationVisible={Boolean(props.globalNotification)}
>
{isClientSide ? (
<SWRConfig value={{ provider: localStorageProvider }}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

#1 I wonder if something like the SWRConfig would not be better placed in gatsby-browser's "wrapXXXElement" hooks. That implicitly makes them client side only and also makes sure they are in all layouts.

#2 having one cache provider for any SWR even across microsites felt weird to me too in the beginning but I concluded that it's the whole point of it. The cache keys are the URLs you use to fetch from SWR and the content is the API response. And we want that information to be available at least in stale state for other pages / views / microsites. So it must be one fixed key like "app-cache" and also at top level for the domain. We could use a more specific localstorage key maybe that is more explanatory.

@gabriele-ct
Copy link
Contributor Author

The behavior is nice :-)

Still a slight delay but that is the general hydration of the static page after reloading (it just becomes more visible but is not new, the top meny does not work too in the first second).

If you have time I'd like you to have a look to more specific scenarios like, for example:

  • new user signup
  • takes part of the course
  • logs out
  • logs in again
  • take another quiz

I felt something was wrong, but I might be wrong

@nkuehn
Copy link
Collaborator

nkuehn commented Apr 5, 2023

If you have time I'd like you to have a look to more specific scenarios like, for example:

@FFawzy could you do me a favor and test through the mentioned scenarios with a diligent look at the status indicators in the navigation? Realistically I would not be able to follow up because I'll only be there for a day next week.

@FFawzy
Copy link
Contributor

FFawzy commented Apr 11, 2023

Notes while testing the preview:

  • if we don't have anything stored in the local storage and then you take a quiz and pass it, there is a green tick appears but then if you navigate to a different page, the tick disappears for a sec and then appears again (I guess it tries to read from localstorage, does not find it and fetch it again)
  • if we have some data stored in the localstorage it does not seem to get updated properly.
    try taking a quiz, pass it. then refresh the page (storing the data in local storage) then try to take a second quiz and pass it too.
    then try to navigate to a different mdx page within the same site, you will see the second quiz status disappears and appears again,

TBC with more test cases

@nkuehn
Copy link
Collaborator

nkuehn commented Apr 12, 2023

the tick disappears for a sec and then appears again
@FFawzy to properly analyze you have to have the network tab open and filter the api requests. There is a short delay on reloads (like microsite switch) that cannot be avoided because it's the time the browser takes to hydrate the static page with the app. But there could also be a longer delay if it really re-fetches the data. Slow-motion / slow network mode of the dev tools is you friend in this kind of analysis work.
The second topic you brought up also sounds like you need to analyze it in depth with slow mo and the API calls logged while it loads

Base automatically changed from ga-topic-status to main April 18, 2023 15:51
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.

3 participants