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.

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

Reduce react-icons package size #562

Merged
merged 3 commits into from
Apr 3, 2023
Merged

Reduce react-icons package size #562

merged 3 commits into from
Apr 3, 2023

Conversation

kkocdko
Copy link
Contributor

@kkocdko kkocdko commented Mar 19, 2023

Define icons by the new createFluentIcon helper function, to reduce the npm package size and bundle size.

The ./lib dir: 21.3 MiB -> 9.85 MiB.

Issue #561

displayName: string | undefined;
}

export const createFluentIcon = (displayName: string, width: string, paths: string[]): FluentIcon => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Function name is similar to ./src/utils/fonts/createFluentFontIcon helper function.

import { FluentIconsProps } from "./FluentIconsProps.types";
import wrapIcon from "./wrapIcon";

export type FluentIcon = {
Copy link
Contributor Author

@kkocdko kkocdko Mar 19, 2023

Choose a reason for hiding this comment

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

Also reduce the size of .d.ts. If not, every icon will produce very long and same type define.

Prefer to use type instead of interface here because it's more transparent and avoid breaking changes in some extreme case.

Copy link
Member

Choose a reason for hiding this comment

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

That's great improvement, it's what I also mentioned when was checking this area.

However, I think that it could be improved even more. In this PR we have:

export declare const Accessibility16Regular: import("../utils/createFluentIcon").FluentIcon;
export declare const Accessibility20Filled: import("../utils/createFluentIcon").FluentIcon;
export declare const Accessibility20Regular: import("../utils/createFluentIcon").FluentIcon;

But we can do following:

export declare const AccessibilityRegular: FluentIcon;
export declare const AccessibilityCheckmarkFilled: FluentIcon;
export declare const AccessibilityCheckmarkRegular: FluentIcon;

We need to do following changes to achieve that:

  for(const chunk of iconChunks) {
    chunk.unshift(`import { createFluentIcon } from "../utils/createFluentIcon";`);
+   chunk.unshift(`import type { FluentIcon } from "../utils/createFluentIcon";`);
  }
-      var jsCode = `export const ${destFilename} = /*#__PURE__*/createFluentIcon('${destFilename}', ${width}, [${paths}]);`
+      var jsCode = `export const ${destFilename}: FluentIcon = /*#__PURE__*/createFluentIcon('${destFilename}', ${width}, [${paths}]);`

@@ -1,10 +1,8 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

const svgr = require("@svgr/core");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove svgr because now we only want to extract data but never transform the SVG.

@kkocdko

This comment was marked as resolved.

@kkocdko

This comment was marked as resolved.

@layershifter
Copy link
Member

@kkocdko we have an issue on Fluent UI side related to this problem, microsoft/fluentui#25989. The idea with modifying glyphs is pretty nice. However, it requires more checks to ensure that nothing else will regress.

Can you please split this PR to two separate ones? I.e. make types changes and code changes separate. With that we could progress faster.

@layershifter
Copy link
Member

BTW, while I can agree about dist size improvements, improvements in bundle size are not so great 🐱

┌────────────────────────────────┬───────────────┬───────────┐
│ Fixture                        │ Minified size │ GZIP size │
├────────────────────────────────┼───────────────┼───────────┤
│ react-icons (new): single icon │ 5.844 kB      │ 2.659 kB  │
├────────────────────────────────┼───────────────┼───────────┤
│ react-icons (new): 5 icons     │ 7.765 kB      │ 3.041 kB  │
├────────────────────────────────┼───────────────┼───────────┤
│ react-icons (old): single icon │ 5.726 kB      │ 2.603 kB  │
├────────────────────────────────┼───────────────┼───────────┤
│ react-icons (old): 5 icons     │ 8.541 kB      │ 3.028 kB  │
└────────────────────────────────┴───────────────┴───────────┘

@kkocdko kkocdko closed this Mar 30, 2023
@kkocdko kkocdko reopened this Mar 30, 2023
@kkocdko
Copy link
Contributor Author

kkocdko commented Mar 30, 2023

@layershifter

About .d.ts file

Update: Fixed. I just remove : FluentIcon and keep import type { FluentIcon }. But don't know why.

I also tried your suggesstion:

for(const chunk of iconChunks) {
    chunk.unshift(`import { createFluentIcon } from "../utils/createFluentIcon";`);
+   chunk.unshift(`import type { FluentIcon } from "../utils/createFluentIcon";`);
}

-      var jsCode = `export const ${destFilename} = /*#__PURE__*/createFluentIcon('${destFilename}', ${width}, [${paths}]);`
+      var jsCode = `export const ${destFilename}: FluentIcon = /*#__PURE__*/createFluentIcon('${destFilename}', ${width}, [${paths}]);`

But what I shocked is, if I import the type FluentIcon and write export const xxx: FluentIcon, the /*#__PURE__*/ mark will gone in bundle result! I don't know why and how to fix this. Now fixed and your suggesstion applied.

Split this PR to two separate ones

Maybe hard to do... Create a type FluentIcon in pr 1 and use it in pr 2 will cause build failed, looks unpleasant. And I think this pr with < 40 lines of code should not be split more.

About bundle size in less-icon project

Thanks for you mention, I optimized the createFluentIcon function, please test again.

@@ -1,7 +1,7 @@
{
"compilerOptions": {
"outDir": "lib",
"target": "ES6",
"target": "esnext",
"module": "esnext",
"lib": ["es6", "dom"],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reduce some size. Because we already use "module": "esnext" before, so no breaking changes.

@layershifter
Copy link
Member

But what I shocked is, if I import the type FluentIcon and write export const xxx: FluentIcon, the /*#__PURE__*/ mark will gone in bundle result! I don't know why and how to fix this. Now fixed and your suggesstion applied.

Oh my gosh, this TS bug strikes again, it's fixed in newer versions of TS 🙈

@kkocdko you need to wrap it with parens:

var jsCode = `export const ${destFilename}: FluentIcon = (/*#__PURE__*/createFluentIcon('${destFilename}', ${width}, [${paths}]));`

Here is a repro.

@layershifter
Copy link
Member

Maybe hard to do... Create a type FluentIcon in pr 1 and use it in pr 2 will cause build failed, looks unpleasant. And I think this pr with < 40 lines of code should not be split more.

Well, I personally looking for a bigger refactor 🐱

As the chain is strange now: createFluentIcon() => wrapIcon()=> useIconState(). For me, it looks like too much for an icon 🤔 However, it has nothing to do with your PR.

I don't have other comments to this PR, @kkocdko great job ⭐ If you would like to contribute more, let's continue on microsoft/fluentui#25989.

@kkocdko
Copy link
Contributor Author

kkocdko commented Apr 3, 2023

@layershifter wrapIcon() is skipped in createFluentIcon() but the define is not removed to avoid breaking changes.

Feel free to merge, close or reuse the code in this PR.

sopranopillow
sopranopillow previously approved these changes Apr 3, 2023
Co-authored-by: Oleksandr Fediashov <[email protected]>
@tomi-msft tomi-msft merged commit e488e78 into microsoft:main Apr 3, 2023
@tomi-msft
Copy link
Contributor

Thanks for your contribution! This was extremely helpful

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants