-
Notifications
You must be signed in to change notification settings - Fork 8.6k
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
perf: using-dynamic-imports #19204
base: main
Are you sure you want to change the base?
perf: using-dynamic-imports #19204
Conversation
@TusharBhatt1 is attempting to deploy a commit to the cal-staging Team on Vercel. A member of the Team first needs to authorize it. |
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details:
|
@@ -157,8 +188,7 @@ function BookingsContent({ status }: BookingsProps) { | |||
]; | |||
}, [user, status]); | |||
|
|||
const isEmpty = useMemo(() => !query.data?.pages[0]?.bookings.length, [query.data]); |
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.
Its computation is inexpensive and doesn't impact performance.
Introduces overhead by adding complexity and memory usage for tracking dependencies
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (02/09/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add community label" took an action on this PR • (02/09/25)1 label was added to this PR based on Keith Williams's automation. |
|
||
import useMeQuery from "@lib/hooks/useMeQuery"; | ||
|
||
import BookingListItem from "@components/booking/BookingListItem"; |
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.
Booking List Item is core part of /bookings. I don't think we should dynamically load this as this component would be required 99% of the times
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 understood your concern , but there are cases when the user has no bookings or an error occurs, making this component unnecessary. Moreover, we’re dynamically importing it on the server, so it won’t impact client-side performance.
Also it's a heavy component , dynamic import will be beneficial
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'm not too sure about this. As Udit said, it's 99% of the times required. Reducing the bundle size on the server side won't affect much.
In the end, we download the same size of the bundle on the client side (because we need them).
But with this approach, the client will have to make an asynchronous request to get these lazy components which is unnecessary separate requests with unncessary loaders.
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.
Understood , but use cases where the component is required, the additional request overhead is negligible compared to the performance gains for the edge cases.
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 don't understand how this is a performance gain. DataTable and BookingListItem is an essential part of this page. Even if the size of page.js is reduced, we immediately load them in separate requests.
On the other hand, I think it's indeed a performance win for WipeMyCalActionButton
because it's clearly "optionally" used after a certain amount of time of the page load.
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 understand your perspective, but the performance gain comes from deferring the loading of DataTable and BookingListItem
(in times when no bookings , error , etc ) until they are actually needed, rather than including them in the initial page payload.
While it's true that these components are eventually required and are loaded in separate requests, the initial page load benefits because :
- The size of page.js is reduced, which speeds up the Time to First Byte (TTFB) and the rendering of the critical parts of the page.
- Components like BookingListItem and DataTable are dynamically loaded (only when required , not everytime during initial page load), so their loading happens in parallel with other resources instead of blocking the page render.
This approach is particularly effective because these components aren’t required for every use case (e.g., when there are no bookings or an error occurs). By deferring their loading, we reduce unnecessary server load and improve the perceived load time for the end user.
Let me know if I am missing something
const DataTableWrapper = dynamic( | ||
() => import("@calcom/features/data-table").then((mod) => mod.DataTableWrapper), | ||
{ | ||
ssr: false, | ||
loading: () => <SkeletonLoader />, | ||
} | ||
); | ||
|
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.
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 understood your concern , but there are cases when this won't be required like if the query fails, bookings are empty, or there are no upcoming bookings today.
const FiltersContainer = dynamic( | ||
() => import("@calcom/features/bookings/components/FiltersContainer").then((mod) => mod.FiltersContainer), | ||
{ | ||
ssr: false, | ||
loading: () => <Icon name="loader" className="animate-spin" />, | ||
} | ||
); | ||
|
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.
Screen.Recording.2025-02-10.at.2.12.20.PM.mov
We should show a skeleton loader here instead of this loader
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.
Sure
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 think it's degrading the user experience by showing a loader when toggling filter components. It should already be there.
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.
Understood , but based on my analysis, dynamic import is the ideal approach for this scenario.
During the initial page load this is not required and there are high possibility that the user doesn’t require the component , such as when they don’t interact with the filter.
Additionally, we are already dynamically importing DateRangePicker
, the user is still experiencing a very minimal delay. However, the performance gain from this approach is genuine and ensures we only load what’s necessary.
import dynamic from "next/dynamic";
export const DateRangePickerLazy = dynamic(() =>
import("./DateRangePicker").then((mod) => mod.DatePickerWithRange)
);
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.
In case of the DateRangePicker, it's obvious to dynamically load it, because
1. Page Load > 2. User wants to filter > 3. User wants to filter by custom date range
It's at the 3rd step in the funnel. However, FiltersContainer
is the 2nd step. So it's a different story.
Can we get the specific number of bundle size different by dynamically importing FiltersContainer
?
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.
290kB (transferred over network) and resource size is 1.3 MB
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.
Screen.Recording.2025-02-10.at.2.12.20.PM.mov
We should show a skeleton loader here instead of this loader
Done
…om/TusharBhatt1/cal.com into perf/dynamic-imports-bookings-page
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.
Type checks are failing
E2E results are ready! |
Fixed (branch update was required) |
What does this PR do?
This PR optimizes the
/bookings
page by adding dynamic imports toreduce bundle size by around 75%
and improve performance.Components imported dynamically , since they are rendered conditionally :
1. Alert (@calcom/ui)
2. DataTableWrapper (@calcom/features/data-table)
3. BookingListItem (@components/booking/BookingListItem)
4. EmptyScreen (@calcom/ui)
5. WipeMyCalActionButton (@calcom/app-store/wipemycalother/components)
6. FiltersContainer (@calcom/features/bookings/components/FiltersContainer)
Adds loading indicators for a smoother user experience while components load.
Disables SSR for specific components to save resources and focus on client-side rendering.
Dynamic import (import() in Next.js) loads modules only when needed, reducing the initial bundle size and improving performance by enabling code-splitting and lazy loading.
Before :
After :
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Tested , works as expected.