-
Notifications
You must be signed in to change notification settings - Fork 143
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
[BUG] 🪳 Fix unwanted re-fetching of data on first page load #1912
Conversation
packages/template-retail-react-app/app/components/_app-config/index.jsx
Outdated
Show resolved
Hide resolved
packages/template-retail-react-app/app/components/_app-config/index.jsx
Outdated
Show resolved
Hide resolved
beforeHydrate: (data) => { | ||
const now = Date.now() | ||
|
||
// Helper for removing the data timestamp. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is no longer true.
// Helper for removing the data timestamp. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the comment
preloadedState = beforeHydrate(window.__PRELOADED_STATE__?.[STATE_KEY] || {}) | ||
} catch (e) { | ||
logger.error('Client `beforeHydrate` failed', { | ||
namespace: 'WithReactQuery render', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's have the namespace match our convention... more code like (e.g. no spaces, but can have dots). For example, see
logger.error(cause, {namespace: 'react-rendering.render'}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the logger to use with-react-query.render
packages/pwa-kit-react-sdk/src/ssr/universal/components/with-react-query/index.test.js
Show resolved
Hide resolved
@@ -29,6 +39,49 @@ describe('withReactQuery', function () { | |||
expect(screen.getByText(/Hello world/i)).toBeInTheDocument() | |||
}) | |||
|
|||
test('`beforeHydrate` called on mount', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have tests for the beforeHydrate
. Can we also have a test on the retail-react-app too?
Say for a page like PLP, have a test to verify that there is no skeleton and no productSearch refetch. Otherwise, if there's a regression in this behaviour, we can only detect it manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we currently have unit test for our a page components, and how those are done is purely run in a client-side manner (e.g. the server side data fetching and rendering is not built into the unit tests). We do have E2E tests that would probably be a better place to home the requests test. Unfortunately all our E2E tests don't access the PLP directly, they simply do things like guest flow and registered flow, meaning the PLP is always a soft navigation.
How married are you to having a one off e2e test to check that skeletons don't show up. It's not a bad idea, but I know we have been talking about having a separate push for uplifting out e2e tests and that might be a goos AC for that work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've created a ticket to create e2e on a broader scope for testing hydration state across the template.
Description
Currently the
template-retail-react-app
uses astaleTime
of 10 seconds for React Query. We use this value, in part, to avoid re-fetching data on the first page load.Unfortunately we have not accounted for cached pages. This means a cached response can be up to 10 minutes old (by default) and the data within the cached responses is almost always stale unless the cached pages is less that 10 seconds old.
This isn't the behaviour we want or had intended. We want the data to be fresh for the 10 seconds as defined in the stale time prop. Meaning the data's
dataUpdatedAt
should always be the same time the app loads.In this PR we ensure that we "fudge" the data's timestamp and assign it the datetime that the app of loaded, not the value it was initially fetched.
NOTE: This isn't a great solution so I'm looking for some feedback, below are some alternative solutions that I've looked in to or considered.
Screen.Recording.2024-07-19.at.11.47.54.AM.-.2.mov
Lighthouse Scores
Before:
After:
Note: Score values seemed to be more unpredictable on the pre-fix and were all over the place (probably as sometimes the loading skeleton was shown and sometimes not) on the fixed version they were more consistent.
Types of Changes
Changes
How to Test-Drive This PR
fast 3g
(maybe evenslow 3g
)Checklists
General