Skip to content

chore: decoupled svg imports from main bundle #36662

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

Merged
vsvamsi1 merged 3 commits into from
Oct 5, 2024
Merged

chore: decoupled svg imports from main bundle #36662

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ad6bed2
decoupled svg imports from the main bundle.js 830KB data is removed f…
vsvamsi1 Oct 2, 2024
cc195fb
fixed test case to check during page load the preloaded chunks are lo…
vsvamsi1 Oct 3, 2024
c6ee0c3
prettier fix
vsvamsi1 Oct 4, 2024
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
19 changes: 7 additions & 12 deletions app/client/cypress/e2e/Regression/ClientSide/Performance/LinkRelPreload_Spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,15 +112,15 @@ function testPreloadMetadata(viewOrEditMode) {
(link) => (window.CDN_URL ?? "/") + link,
);

const requestsToCompare = unique(
const allRequestsDuringPageLoad = unique(
jsRequests.filter(
(link) =>
// Exclude link bundle requests. We don’t really care about being precise
// with their preloading, as icons aren’t critical for the first paint
!link.includes("-icons."),
),
);
const linksToCompare = unique(
const preloadLinks = unique(
links.filter(
(link) =>
// Exclude link bundle preloads. We don’t really care about being precise
Expand All @@ -132,16 +132,11 @@ function testPreloadMetadata(viewOrEditMode) {
),
);

const actuallyLoadedFiles = `[${
requestsToCompare.length
} items] ${requestsToCompare.sort().join(", ")}`;
const preloadedFiles = `[${linksToCompare.length} items] ${linksToCompare
.sort()
.join(", ")}`;

// Comparing strings instead of deep-equalling arrays because this is the only way
// to see which chunks are actually missing: https://github.com/cypress-io/cypress/issues/4084
cy.wrap(actuallyLoadedFiles).should("equal", preloadedFiles);
// check if req
const isSubset = preloadLinks.every((item) =>
allRequestsDuringPageLoad.includes(item),
);
expect(isSubset).to.be.true;
});
}

Expand Down
72 changes: 57 additions & 15 deletions app/client/src/components/designSystems/blueprintjs/icon/Icon.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
// See readme.md for why this file exists.

import React, { useMemo } from "react";
import React, {
useEffect,
useState,
type ComponentType,
type SVGProps,
} from "react";
import classNames from "classnames";
import type { IconProps } from "@blueprintjs/core";
import svgImportsMap from "components/designSystems/blueprintjs/icon/svgImportsMap";
// Below symbols must be imported directly from target files to avoid crashes
// caused by cyclic dependencies in @blueprintjs/core.
import {
Expand All @@ -12,12 +16,34 @@ import {
intentClass,
} from "@blueprintjs/core/lib/esm/common/classes";
import { importSvg } from "@appsmith/ads-old";
import type { IconName } from "@blueprintjs/core";

// This export must be named "IconSize" to match the exports of @blueprintjs/core/lib/esm/components/icon
export enum IconSize {
STANDARD = 16,
LARGE = 20,
}
type IconMapType = Record<
IconName,
// eslint-disable-next-line @typescript-eslint/consistent-type-imports
Record<IconSize, () => Promise<typeof import("*.svg")>>
>;

// Create a variable to cache the imported module
let cachedSvgImportsMap: IconMapType | null = null;

// Function to lazily load the file once
const loadSvgImportsMapOnce = async () => {
if (!cachedSvgImportsMap) {
const { default: svgImportsMap } = await import(
"components/designSystems/blueprintjs/icon/svgImportsMap"
);

cachedSvgImportsMap = svgImportsMap; // Cache the module for future use
}

return cachedSvgImportsMap;
};

function Icon(props: IconProps) {
const {
Expand All @@ -31,18 +57,32 @@ function Icon(props: IconProps) {
tagName: TagName = "span",
...htmlprops
} = props;
const [SvgIcon, setSvgIcon] = useState<ComponentType<
SVGProps<SVGSVGElement>
> | null>(null);

// choose which pixel grid is most appropriate for given icon size
const pixelGridSize =
size >= IconSize.LARGE ? IconSize.LARGE : IconSize.STANDARD;

// render the icon, or nothing if icon name is unknown.
const SvgIcon = useMemo(() => {
if (typeof icon === "string" && icon in svgImportsMap) {
return importSvg(svgImportsMap[icon][pixelGridSize]);
}
useEffect(() => {
const loadScript = async () => {
// Load the cached svgImportsMap once
const svgImportsMap = await loadSvgImportsMapOnce();

if (typeof icon === "string" && icon in svgImportsMap) {
const SvgIcon = await importSvg(svgImportsMap[icon][pixelGridSize]);

setSvgIcon(() => SvgIcon); // Set the component as lazy-loaded
}
};

loadScript();

return () => null;
// Cleanup on unmount
return () => {
setSvgIcon(null);
};
Comment on lines +68 to +85
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 2, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Let's enhance error handling in asynchronous operations

When dealing with asynchronous functions like loadScript, it's important to anticipate potential errors that might occur during the loading process. To make our code more robust, consider adding a try-catch block to handle any exceptions gracefully.

Here's how you might modify the loadScript function:

useEffect(() => {
  const loadScript = async () => {
+   try {
      // Load the cached svgImportsMap once
      const svgImportsMap = await loadSvgImportsMapOnce();

      if (typeof icon === "string" && icon in svgImportsMap) {
        const SvgIcon = await importSvg(svgImportsMap[icon][pixelGridSize]);

        setSvgIcon(() => SvgIcon); // Set the component as lazy-loaded
      }
+   } catch (error) {
+     console.error("Error loading SVG icon:", error);
+     // You might set a default icon or handle the error state here
+     setSvgIcon(null);
+   }
  };

  loadScript();

  // Cleanup on unmount
  return () => {
    setSvgIcon(null);
  };
}, [icon, pixelGridSize]);

By adding error handling, we ensure that our application remains stable even if an icon fails to load. This will enhance the user experience and prevent unforeseen issues.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
useEffect(() => {
const loadScript = async () => {
// Load the cached svgImportsMap once
const svgImportsMap = await loadSvgImportsMapOnce();
if (typeof icon === "string" && icon in svgImportsMap) {
const SvgIcon = await importSvg(svgImportsMap[icon][pixelGridSize]);
setSvgIcon(() => SvgIcon); // Set the component as lazy-loaded
}
};
loadScript();
return () => null;
// Cleanup on unmount
return () => {
setSvgIcon(null);
};
useEffect(() => {
const loadScript = async () => {
try {
// Load the cached svgImportsMap once
const svgImportsMap = await loadSvgImportsMapOnce();
if (typeof icon === "string" && icon in svgImportsMap) {
const SvgIcon = await importSvg(svgImportsMap[icon][pixelGridSize]);
setSvgIcon(() => SvgIcon); // Set the component as lazy-loaded
}
} catch (error) {
console.error("Error loading SVG icon:", error);
// You might set a default icon or handle the error state here
setSvgIcon(null);
}
};
loadScript();
// Cleanup on unmount
return () => {
setSvgIcon(null);
};
}, [icon, pixelGridSize]);

Comment on lines +60 to +85
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 3, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Time for a pop quiz on asynchronous operations!

Now, class, let's look at how we're loading our SVG icons. We're using some fancy React hooks called useState and useEffect. Can anyone tell me what these do?

Very good! useState helps us remember things, and useEffect helps us do things at the right time.

But wait a minute... I see something missing. Can anyone spot it? That's right, we're not handling errors! What if something goes wrong when we're trying to load our icons?

Let's improve this, shall we? Here's a little homework for you:

useEffect(() => {
  const loadScript = async () => {
    try {
      const svgImportsMap = await loadSvgImportsMapOnce();

      if (typeof icon === "string" && icon in svgImportsMap) {
        const SvgIcon = await importSvg(svgImportsMap[icon][pixelGridSize]);
        setSvgIcon(() => SvgIcon);
      }
    } catch (error) {
      console.error("Error loading SVG icon:", error);
      setSvgIcon(null);
    }
  };

  loadScript();

  return () => {
    setSvgIcon(null);
  };
}, [icon, pixelGridSize]);

Can you see how we've added a try-catch block? This is like a safety net for our code. If anything goes wrong, we'll know about it and our program won't crash. Always remember to handle your errors, class!

}, [icon, pixelGridSize]);

if (icon == null || typeof icon === "boolean") {
Expand All @@ -68,13 +108,15 @@ function Icon(props: IconProps) {
icon={icon}
title={htmlTitle}
>
<SvgIcon
data-icon={icon}
fill={color}
height={size}
viewBox={viewBox}
width={size}
/>
{SvgIcon && (
<SvgIcon
data-icon={icon}
fill={color}
height={size}
viewBox={viewBox}
width={size}
/>
)}
</TagName>
);
}
Expand Down