-
Notifications
You must be signed in to change notification settings - Fork 9.3k
perf: optimize app loading and rendering performance with CI fix #21052
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
Changes from all commits
6c90699
6cff784
b209383
4ac1fbb
4d3b5fe
fc47e60
6bfc888
6af84ac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,8 @@ | ||
"use client"; | ||
|
||
import type { ChangeEventHandler } from "react"; | ||
import { useState } from "react"; | ||
import { useState, lazy, Suspense } from "react"; | ||
|
||
import { AllApps } from "@calcom/features/apps/components/AllApps"; | ||
import { AppStoreCategories } from "@calcom/features/apps/components/Categories"; | ||
import { PopularAppsSlider } from "@calcom/features/apps/components/PopularAppsSlider"; | ||
import { RecentAppsSlider } from "@calcom/features/apps/components/RecentAppsSlider"; | ||
import { useLocale } from "@calcom/lib/hooks/useLocale"; | ||
import type { AppCategories } from "@calcom/prisma/enums"; | ||
import type { AppFrontendPayload } from "@calcom/types/App"; | ||
|
@@ -18,6 +14,23 @@ import { HorizontalTabs } from "@calcom/ui/components/navigation"; | |
|
||
import AppsLayout from "@components/apps/layouts/AppsLayout"; | ||
|
||
const AllApps = lazy(() => | ||
import("@calcom/features/apps/components/AllApps").then((mod) => ({ default: mod.AllApps })) | ||
); | ||
const AppStoreCategories = lazy(() => | ||
import("@calcom/features/apps/components/Categories").then((mod) => ({ default: mod.AppStoreCategories })) | ||
); | ||
const PopularAppsSlider = lazy(() => | ||
import("@calcom/features/apps/components/PopularAppsSlider").then((mod) => ({ | ||
default: mod.PopularAppsSlider, | ||
})) | ||
); | ||
const RecentAppsSlider = lazy(() => | ||
import("@calcom/features/apps/components/RecentAppsSlider").then((mod) => ({ | ||
default: mod.RecentAppsSlider, | ||
})) | ||
); | ||
|
||
const tabs: HorizontalTabItemProps[] = [ | ||
{ | ||
name: "app_store", | ||
|
@@ -84,18 +97,22 @@ export default function Apps({ isAdmin, categories, appStore, userAdminTeams }: | |
emptyStore={!appStore.length}> | ||
<div className="flex flex-col gap-y-8"> | ||
{!searchText && ( | ||
<> | ||
<AppStoreCategories categories={categories} /> | ||
<PopularAppsSlider items={appStore} /> | ||
<RecentAppsSlider items={appStore} /> | ||
</> | ||
<Suspense fallback={<div className="bg-subtle h-24 animate-pulse rounded-md" />}> | ||
<> | ||
<AppStoreCategories categories={categories} /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. needs a fallback incase of failure other wise will keep showing loader |
||
<PopularAppsSlider items={appStore} /> | ||
<RecentAppsSlider items={appStore} /> | ||
</> | ||
</Suspense> | ||
)} | ||
<AllApps | ||
apps={appStore} | ||
searchText={searchText} | ||
categories={categories.map((category) => category.name)} | ||
userAdminTeams={userAdminTeams} | ||
/> | ||
<Suspense fallback={<div className="bg-subtle h-96 animate-pulse rounded-md" />}> | ||
<AllApps | ||
apps={appStore} | ||
searchText={searchText} | ||
categories={categories.map((category) => category.name)} | ||
userAdminTeams={userAdminTeams} | ||
/> | ||
</Suspense> | ||
</div> | ||
</AppsLayout> | ||
); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,7 @@ | |
"dx": "yarn dev", | ||
"test-codegen": "yarn playwright codegen http://localhost:3000", | ||
"type-check": "tsc --pretty --noEmit", | ||
"type-check:ci": "tsc-absolute --pretty --noEmit", | ||
"type-check:ci": "tsc --pretty --noEmit --skipLibCheck", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. casually changes the entire workflow of the team, for worse well, the builds will be easier for sure There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. insane, closing |
||
"build": "next build", | ||
"start": "next start", | ||
"lint": "eslint . --ignore-path .gitignore", | ||
|
@@ -192,7 +192,7 @@ | |
"tailwindcss": "^3.3.3", | ||
"tailwindcss-animate": "^1.0.6", | ||
"ts-node": "^10.9.1", | ||
"typescript": "^4.9.4" | ||
"typescript": "^4.9.5" | ||
}, | ||
"nextBundleAnalysis": { | ||
"budget": 358400, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
import { appStoreMetadata } from "@calcom/app-store/appStoreMetaData"; | ||
import { getAppFromSlug } from "@calcom/app-store/utils"; | ||
import getInstallCountPerApp from "@calcom/lib/apps/getInstallCountPerApp"; | ||
import { getCache, setCache } from "@calcom/lib/cache"; | ||
import { getAllDelegationCredentialsForUser } from "@calcom/lib/delegationCredential/server"; | ||
import type { UserAdminTeams } from "@calcom/lib/server/repository/user"; | ||
import prisma, { safeAppSelect, safeCredentialSelect } from "@calcom/prisma"; | ||
|
@@ -37,7 +38,11 @@ export async function getAppWithMetadata(app: { dirName: string } | { slug: stri | |
} | ||
|
||
/** Mainly to use in listings for the frontend, use in getStaticProps or getServerSideProps */ | ||
export async function getAppRegistry() { | ||
export async function getAppRegistry(): Promise<App[]> { | ||
const cacheKey = "app-registry"; | ||
const cachedApps = getCache<App[]>(cacheKey); | ||
if (cachedApps) return cachedApps; | ||
|
||
const dbApps = await prisma.app.findMany({ | ||
where: { enabled: true }, | ||
select: { dirName: true, slug: true, categories: true, enabled: true, createdAt: true }, | ||
|
@@ -59,10 +64,25 @@ export async function getAppRegistry() { | |
installCount: installCountPerApp[dbapp.slug] || 0, | ||
}); | ||
} | ||
|
||
setCache(cacheKey, apps, 5 * 60); // Cache for 5 minutes | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shud come from a configuration |
||
return apps; | ||
} | ||
|
||
export async function getAppRegistryWithCredentials(userId: number, userAdminTeams: UserAdminTeams = []) { | ||
type AppWithCredentials = App & { | ||
credentials: Credential[]; | ||
isDefault?: boolean; | ||
dependencyData?: TDependencyData; | ||
}; | ||
|
||
export async function getAppRegistryWithCredentials( | ||
userId: number, | ||
userAdminTeams: UserAdminTeams = [] | ||
): Promise<AppWithCredentials[]> { | ||
const cacheKey = `app-registry-creds-${userId}-${userAdminTeams.join(",")}`; | ||
const cachedApps = getCache<AppWithCredentials[]>(cacheKey); | ||
if (cachedApps) return cachedApps; | ||
|
||
// Get teamIds to grab existing credentials | ||
|
||
const dbApps = await prisma.app.findMany({ | ||
|
@@ -137,5 +157,6 @@ export async function getAppRegistryWithCredentials(userId: number, userAdminTea | |
}); | ||
} | ||
|
||
setCache(cacheKey, apps, 5 * 60); // Cache for 5 minutes | ||
return apps; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
import { memo } from "react"; | ||
|
||
import type { UserAdminTeams } from "@calcom/lib/server/repository/user"; | ||
import type { AppFrontendPayload } from "@calcom/types/App"; | ||
import type { CredentialFrontendPayload } from "@calcom/types/Credential"; | ||
|
||
import { AppCard } from "./AppCard"; | ||
|
||
type MemoizedAppCardProps = { | ||
app: AppFrontendPayload & { credentials?: CredentialFrontendPayload[] }; | ||
searchText?: string; | ||
credentials?: CredentialFrontendPayload[]; | ||
userAdminTeams?: UserAdminTeams; | ||
}; | ||
|
||
export const MemoizedAppCard = memo( | ||
(props: MemoizedAppCardProps) => <AppCard {...props} />, | ||
(prevProps, nextProps) => { | ||
return ( | ||
prevProps.app.slug === nextProps.app.slug && | ||
prevProps.searchText === nextProps.searchText && | ||
prevProps.credentials?.length === nextProps.credentials?.length | ||
); | ||
} | ||
); | ||
|
||
MemoizedAppCard.displayName = "MemoizedAppCard"; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
type CacheEntry<T> = { | ||
value: T; | ||
expiry: number; | ||
}; | ||
|
||
const cache = new Map<string, CacheEntry<unknown>>(); | ||
|
||
/** | ||
* Set a value in the cache with an expiration time | ||
* @param key - Cache key | ||
* @param value - Value to cache | ||
* @param ttlSeconds - Time to live in seconds | ||
*/ | ||
export function setCache<T>(key: string, value: T, ttlSeconds: number): void { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. validate cache size - guess browsers do it now themselves but just in case There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how does cache invalidation work here |
||
const expiry = Date.now() + ttlSeconds * 1000; | ||
cache.set(key, { value, expiry }); | ||
} | ||
|
||
/** | ||
* Get a value from the cache | ||
* @param key - Cache key | ||
* @returns The cached value or null if not found or expired | ||
*/ | ||
export function getCache<T>(key: string): T | null { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. type validation missing |
||
const entry = cache.get(key); | ||
if (!entry) return null; | ||
|
||
if (Date.now() > entry.expiry) { | ||
cache.delete(key); | ||
return null; | ||
} | ||
|
||
return entry.value as T; | ||
} | ||
|
||
/** | ||
* Delete a value from the cache | ||
* @param key - Cache key | ||
*/ | ||
export function deleteCache(key: string): void { | ||
cache.delete(key); | ||
} | ||
|
||
/** | ||
* Clear all cached values | ||
*/ | ||
export function clearCache(): void { | ||
cache.clear(); | ||
} |
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.
If type check is failing, why not remove it?
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.
Lmao