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.

Sign up for GitHub

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

Closed
wants to merge 8 commits into from
Closed

perf: using-dynamic-imports #19204

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
572ac1a
perf : using dynamic imports
TusharBhatt1 Feb 9, 2025
cad7016
revert condtion
TusharBhatt1 Feb 9, 2025
eef2041
fix: ts error
TusharBhatt1 Feb 9, 2025
c800a59
Merge branch 'main' into perf/dynamic-imports-bookings-page
TusharBhatt1 Feb 10, 2025
2627856
Merge branch 'main' into perf/dynamic-imports-bookings-page
TusharBhatt1 Feb 10, 2025
993b334
fix: add skeleton
TusharBhatt1 Feb 10, 2025
841781d
Merge branch 'perf/dynamic-imports-bookings-page' of https://github.c…
TusharBhatt1 Feb 10, 2025
5f75929
Merge branch 'main' into perf/dynamic-imports-bookings-page
TusharBhatt1 Feb 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 39 additions & 8 deletions apps/web/modules/bookings/views/bookings-listing-view.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,29 +7,60 @@ import {
getSortedRowModel,
createColumnHelper,
} from "@tanstack/react-table";
import dynamic from "next/dynamic";
import { useMemo, useState } from "react";
import type { z } from "zod";

import { WipeMyCalActionButton } from "@calcom/app-store/wipemycalother/components";
import dayjs from "@calcom/dayjs";
import { FilterToggle } from "@calcom/features/bookings/components/FilterToggle";
import { FiltersContainer } from "@calcom/features/bookings/components/FiltersContainer";
import type { filterQuerySchema } from "@calcom/features/bookings/lib/useFilterQuery";
import { useFilterQuery } from "@calcom/features/bookings/lib/useFilterQuery";
import { DataTableProvider, DataTableWrapper } from "@calcom/features/data-table";
import { DataTableProvider } from "@calcom/features/data-table";
import { useLocale } from "@calcom/lib/hooks/useLocale";
import type { RouterOutputs } from "@calcom/trpc/react";
import { trpc } from "@calcom/trpc/react";
import type { HorizontalTabItemProps, VerticalTabItemProps } from "@calcom/ui";
import { Alert, EmptyScreen, HorizontalTabs } from "@calcom/ui";
import { HorizontalTabs } from "@calcom/ui";
import { Icon } from "@calcom/ui";

import useMeQuery from "@lib/hooks/useMeQuery";

import BookingListItem from "@components/booking/BookingListItem";
Copy link
Contributor

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

Copy link
Contributor Author

@TusharBhatt1 TusharBhatt1 Feb 10, 2025 •
edited
Loading

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

Copy link
Contributor

@eunjae-lee eunjae-lee Feb 10, 2025 •
edited
Loading

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Screenshot 2025-02-10 at 10 42 55

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.

Copy link
Contributor Author

@TusharBhatt1 TusharBhatt1 Feb 10, 2025 •
edited
Loading

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

import SkeletonLoader from "@components/booking/SkeletonLoader";

import type { validStatuses } from "~/bookings/lib/validStatuses";

const Alert = dynamic(() => import("@calcom/ui").then((mod) => mod.Alert));

const DataTableWrapper = dynamic(
() => import("@calcom/features/data-table").then((mod) => mod.DataTableWrapper),
{
ssr: false,
loading: () => <SkeletonLoader />,
}
);

Comment on lines +33 to +40
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as https://github.com/calcom/cal.com/pull/19204/files#r1948589670.

Copy link
Contributor Author

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 BookingListItem = dynamic(() => import("@components/booking/BookingListItem"), {
ssr: true,
loading: () => <div className="h-20 animate-pulse rounded-md bg-gray-100" />,
});

const EmptyScreen = dynamic(() => import("@calcom/ui").then((mod) => mod.EmptyScreen));

const WipeMyCalActionButton = dynamic(
() => import("@calcom/app-store/wipemycalother/components").then((mod) => mod.WipeMyCalActionButton),
{
ssr: false,
loading: () => <Icon name="loader" className="animate-spin" />,
}
);
const FiltersContainer = dynamic(
() => import("@calcom/features/bookings/components/FiltersContainer").then((mod) => mod.FiltersContainer),
{
ssr: false,
loading: () => <Icon name="loader" className="animate-spin" />,
}
);

Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

Copy link
Contributor

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.

Copy link
Contributor Author

@TusharBhatt1 TusharBhatt1 Feb 10, 2025 •
edited
Loading

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)
);

Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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

type BookingListingStatus = z.infer<NonNullable<typeof filterQuerySchema>>["status"];
type BookingOutput = RouterOutputs["viewer"]["bookings"]["get"]["bookings"][0];

Expand Down Expand Up @@ -157,8 +188,7 @@ function BookingsContent({ status }: BookingsProps) {
];
}, [user, status]);

const isEmpty = useMemo(() => !query.data?.pages[0]?.bookings.length, [query.data]);
Copy link
Contributor Author

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


const isEmpty = !query.data?.pages[0]?.bookings.length;
const flatData = useMemo<RowData[]>(() => {
const shownBookings: Record<string, BookingOutput[]> = {};
const filterBookings = (booking: BookingOutput) => {
Expand Down Expand Up @@ -246,7 +276,7 @@ function BookingsContent({ status }: BookingsProps) {
<HorizontalTabs tabs={tabs} />
<FilterToggle setIsFiltersVisible={setIsFiltersVisible} />
</div>
<FiltersContainer isFiltersVisible={isFiltersVisible} />
{isFiltersVisible && <FiltersContainer />}
<main className="w-full">
<div className="flex w-full flex-col">
{query.status === "error" && (
Expand All @@ -259,6 +289,7 @@ function BookingsContent({ status }: BookingsProps) {
<WipeMyCalActionButton bookingStatus={status} bookingsEmpty={isEmpty} />
)}
<DataTableWrapper
// @ts-expect-error this bypasses ts error caused by type inference issues with dynamic imports - nothing major.
table={table}
testId={`${status}-bookings`}
bodyTestId="bookings"
Expand Down
38 changes: 16 additions & 22 deletions packages/features/bookings/components/FiltersContainer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,7 @@ import { Tooltip, Button } from "@calcom/ui";

import { EventTypeFilter } from "./EventTypeFilter";

export interface FiltersContainerProps {
isFiltersVisible: boolean;
}

export function FiltersContainer({ isFiltersVisible }: FiltersContainerProps) {
export function FiltersContainer() {
const [animationParentRef] = useAutoAnimate<HTMLDivElement>();
const { removeAllQueryParams } = useFilterQuery();
const { t } = useLocale();
Expand All @@ -27,23 +23,21 @@ export function FiltersContainer({ isFiltersVisible }: FiltersContainerProps) {

return (
<div ref={animationParentRef}>
{isFiltersVisible ? (
<div className="no-scrollbar flex w-full space-x-2 overflow-x-scroll rtl:space-x-reverse">
<PeopleFilter />
<EventTypeFilter />
<TeamsFilter />
<StartTimeFilters />
<Tooltip content={t("remove_filters")}>
<Button
disabled={!hasValidQueryParams}
color="secondary"
type="button"
onClick={removeAllQueryParams}>
{t("remove_filters")}
</Button>
</Tooltip>
</div>
) : null}
<div className="no-scrollbar flex w-full space-x-2 overflow-x-scroll rtl:space-x-reverse">
<PeopleFilter />
<EventTypeFilter />
<TeamsFilter />
<StartTimeFilters />
<Tooltip content={t("remove_filters")}>
<Button
disabled={!hasValidQueryParams}
color="secondary"
type="button"
onClick={removeAllQueryParams}>
{t("remove_filters")}
</Button>
</Tooltip>
</div>
</div>
);
}
Loading