-
Notifications
You must be signed in to change notification settings - Fork 0
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
chore: Improve home performance #45
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Preview is readyThis pull request generated a Preview👀 Preview: https://preview-45--nextjs.preview.vtex.app |
551fccf
to
ff88122
Compare
Lighthouse ReportsHere are the Lighthouse reports of this Pull Request📝 Based on commit eeec626
|
ff88122
to
37f848c
Compare
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.
The new stats look promising!
Any ideas on the PDP error logged to the console?
2dd778c
to
6948521
Compare
6948521
to
37f848c
Compare
@filipewl No errors at PDP, take a look there again. Also the last performance tests before - https://sfj-7de89f9--nextjs.preview.vtex.app/
after - https://sfj-37f848c--nextjs.preview.vtex.app/
|
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.
LGTM!
37f848c
to
98a8897
Compare
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.
LGTM. Can you explain how this differs from your previous PR?
Which one? I opened many. But know I'm using the next/dynamic, which seems to work well with the SSR. |
f7acaa3
to
ed80f12
Compare
{ | ||
...options, | ||
fallbackData: | ||
options?.suspense && !options.fallbackData |
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.
the explanation why suspense + ssr need fallback data.
https://github.com/vercel/swr/blob/main/core/use-swr.ts#L567-L580
{ ...options, fetchOptions: { ...options?.fetchOptions, method: 'POST' } } | ||
{ | ||
...options, | ||
fallbackData: |
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.
the explanation why suspense + ssr need fallback data.
https://github.com/vercel/swr/blob/main/core/use-swr.ts#L567-L580
Before - https://sfj-d8224eb--nextjs.preview.vtex.app/
After - https://sfj-da42147--nextjs.preview.vtex.app/
|
@@ -3,7 +3,7 @@ import usePersonQuery from 'src/sdk/person/usePersonQuery' | |||
import { ButtonLink } from 'src/components/ui/Button' | |||
|
|||
const ButtonSignIn = () => { | |||
const person = usePersonQuery() | |||
const person = usePersonQuery({ suspense: true }) |
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.
afterward, we can think about bringing the information of the logged user from the server-side (if the user is logged or not), and so maybe we don't need the additional request on the ButtonSignIn
nor the Skeleton/Loading state since we will have this information on the first render already, like doing the request on the getServerSideProps. What do you think?
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.
Yeah, but by doing this we couldn't leverage from use the same page for all users, because now they vary based on logged users
This reverts commit 46ab248166ac331325622d4b2fed1b4952d0f2c0.
d611e1a
to
5694718
Compare
What's the purpose of this pull request?
The main goal of this PR is to decrease the Total Blocking Time (TBT), and improve the LH score metric.
How does it work?
This PR decreases the highest portion of the TBT from the first render, which is related to execution and compiling code from SWR, ButtonSignIn, two ProductShelf, and ProductTiles. To tackle this problem these components that use the SWR, now pass the
suspense
property to SWR. By doing this these components are suspended and rendered into a new task, decreasing the time from the first render task, consequently decreasing the TBT. You can see this at the flamegraph belowAlso, this PR creates a new task for the SWR fetcher function to decrease the possibility to create long tasks. This change impacts all pages.
Take a look at the results below:
Before - https://sfj-d8224eb--nextjs.preview.vtex.app/
After - https://sfj-da42147--nextjs.preview.vtex.app/
How to test it?
References
https://nextjs.org/docs/advanced-features/dynamic-import
https://loadable-components.com/docs/loadable-vs-react-lazy/
Checklist
You may erase this after checking them all ;)
Added an entry in the
CHANGELOG.md
at the beginning of its due section. The latest version should comes first.Added the PR number with the PR link at the entry in the
CHANGELOG.md
. E.g., New items in thepull_request_template.md
(#4)PR description
Updated the Storybook - if applicable.
Added a label according to the PR goal -
Breaking change
,Enhancement
,Bug
orChore
.Added the component, hook, or pathname in-between backticks (``) - If applicable. E.g.,
ComponentName
component.Identified the function or parameter in the PR - If applicable. E.g.,
useWindowDimensions
hook.For documentation changes, ping @carolinamenezes or @Mariana-Caetano to review and update the changes.