microsoft / fluentui

Fluent UI web represents a collection of utilities, React components, and web components for building web applications.
https://react.fluentui.dev
Other
18.51k stars 2.73k forks source link

@fluentui/react-icons: build performance & package bloat #25989

Open layershifter opened 1 year ago

layershifter commented 1 year ago

Library

React Components / v9 (@fluentui/react-components) & @fluentui/react-icons.

Current state with tooling

Webpack

In development

To be done

In production

Tree-shaking works ✅ Bundles reasonably fast (webpack 5.75.0 on Apple M1 in 1857 ms, 2048 ms) ✅

CodeSandbox

non cloud

Works surprisingly well (even with 4x CPU slowdown): https://codesandbox.io/s/naughty-paper-14wujx?file=/src/App.js

Uses CJS and loads all chunks for every icon ⚠️

image

cloud beta

Works even better https://codesandbox.io/p/sandbox/priceless-knuth-m8e6iz

Loads bundled ESM by Vite (around 2.1mb)

image

TypeScript

Used TS 4.9.4. Builds a fixture reasonably fast with skipLibCheck: true & skipLibCheck: false

Next.js

Used Next.js 13.0.6. Rebuilds in dev are reasonably fast. Builds, too (on Apple M1 12.37s with icons, 8.67s without).

create-react-app

In development

To be done

In production

It's still Webpack under hood, but they process node_modules with Babel loader 💥

Jest

Does not seem that icons affect it in any way.

Package size

It's where things are getting worse. react-icons-2.0.190.tgz downloadable size is 15.1mb, unpacked size is 71mb 💥

Fonts

It's a big problem actually as it double package twice.

Action 1 (mandatory): glyphs should be present once

Currently glyphs are present twice for CJS & ESM 💣 However, fonts are not modules and a JSON file next them is also not a module.

https://www.jsdelivr.com/package/npm/@fluentui/react-icons?path=lib%2Futils%2Ffonts https://www.jsdelivr.com/package/npm/@fluentui/react-icons?path=lib-cjs%2Futils%2Ffonts

If we will have a single instance we go down to 63mb (-8mb!) unpacked.

Action 2: remove fonts completely

Move fonts and related JS to a separate package:

While the plugin looks on a relative path - I don't think that it's a good enough reason to keep ghyphs and slow down installs for everyone.

Deleting fonts completely moves us down to 41mb.

Types

Duplicate types

Currently we have separate types for ESM and CJS:

image

It means that we duplicate definitions for a no reason as the package.json references only lib/index.d.ts and we have export maps that prevent deep imports.

Duplicate types cost 5mb. Ideally, it would be great to rollup them.

Expanded definitions ✅

image

We have this for every icon, types are inlined, not great at all.

Killing a generic there makes types more accurate:

export declare const HeartRegular: {
    (props: FluentIconsProps): JSX.Element;
    displayName: string | undefined;
};

Ideally, it should be just:

export declare const HeartRegular: FluentIcon;

Usage of a type without displayName probably will lead to better results. Just the change above saves 3mb.

Minify output

To be done.

Requested priority

High

layershifter commented 1 year ago

The problem with types affects also consumer side. In the project we have the whole set of icons bundled:

import {
  bundleIcon,
  AccessTimeFilled,
  AccessibilityFilled
} from "@fluentui/react-icons";

export const AccessTime: FluentIcon = /*#__PURE__*/ bundleIcon(
  AccessTimeFilled,
  AccessTimeRegular
);

/* all other icons */
/* ... */

The problem what we have is build performance: we use TS compiler to compile TS code and generate type definitions. tsc --extendedDiagnostics gives following:

  ✖ Files:                         755
  ✖ Lines of Library:            32995
  ✖ Lines of Definitions:       384954
  ✖ Lines of TypeScript:         12437
  ✖ Lines of JavaScript:             0
  ✖ Lines of JSON:                   0
  ✖ Lines of Other:                  0
  ✖ Nodes of Library:           149996
  ✖ Nodes of Definitions:      1566104
  ✖ Nodes of TypeScript:         31750
  ✖ Nodes of JavaScript:             0
  ✖ Nodes of JSON:                   0
  ✖ Nodes of Other:                  0
  ✖ Identifiers:                509267
  ✖ Symbols:                   1071185
  ✖ Types:                       31575
  ✖ Instantiations:            3317977
  ✖ Memory used:               928504K
  ✖ Assignability cache size:    41352
  ✖ Identity cache size:           309
  ✖ Subtype cache size:            430
  ✖ Strict subtype cache size:       0
  ✖ I/O Read time:               0.30s
  ✖ Parse time:                  1.21s
  ✖ ResolveModule time:          0.19s
  ✖ ResolveTypeReference time:   0.07s
  ✖ Program time:                1.84s
  ✖ Bind time:                   0.53s
  ✖ Check time:                  6.67s
  ✖ printTime time:             65.60s
  ✖ Emit time:                  65.60s
  ✖ transformTime time:         65.31s
  ✖ Source Map time:             0.01s
  ✖ commentTime time:            0.03s
  ✖ I/O Write time:              0.05s
  ✖ Total time:                 74.64s

Time printTime, Emit time & transformTime are insane 💥

Running with emitDeclarationOnly if printTime is high. https://github.com/microsoft/TypeScript/wiki/Performance#performance-considerations

I run the same build with declaration: false (which we can't really do):

  ✖ Check time:                  7.33s
  ✖ printTime time:              0.30s
  ✖ Emit time:                   0.31s
  ✖ transformTime time:          0.04s
  ✖ Total time:                 12.18s

At the same time I tried to collect CPU profile to understand what explodes:

Current state

image

declaration: false

image

getAccessibleSymbolChainFromSymbolTable points to https://github.com/microsoft/TypeScript/issues/34119 where the similar issue is discussed: performance slowdown on huge types. Considering that I tried to simplify types to avoid complex d.ts like this:

image

Basically did explicit typing to FluentIcon:

+import * as React from "react";

+type FluentIcon = React.FC<
+  React.SVGAttributes<SVGElement> & {
+    primaryFill?: string | undefined;
+    className?: string | undefined;
+    filled?: boolean | undefined;
+    title?: string | undefined;
+  }
+>;

-export const AccessTime = /*#__PURE__*/ bundleIcon(
+export const AccessTime: FluentIcon = /*#__PURE__*/ bundleIcon(
  AccessTimeFilled,
  AccessTimeRegular
);

The change results in:

image

image

  ✖ printTime time:              0.30s
  ✖ Emit time:                   0.31s
  ✖ transformTime time:          0.10s
  ✖ Source Map time:             0.01s
  ✖ commentTime time:            0.02s
  ✖ I/O Write time:              0.03s
  ✖ Total time:                  8.49s

I am not sure if better typings for icons on lib side will change things, but it would be great to test.

layershifter commented 1 year ago

Other possible actions:

Compress output

Apply minifications to distributed files to compress JS output i.e. run Terser/SWC on lib/**/*.js and lib-cjs/**/*.js.

Do not ship ESM

Another risky attempt might be to ship only CommonJS, here is a sandbox: https://stackblitz.com/edit/node-e7ki6x?file=index.js

While it works with Node:

~/projects/node-e7ki6x
❯ node index.mjs
[Module: null prototype] { FooIcon: 'Foo-Icon', BarIcon: 'Bar-Icon' }

~/projects/node-e7ki6x
❯ node index.cjs
{ FooIcon: 'Foo-Icon', BarIcon: 'Bar-Icon' }

I am not sure what will be an effect on bundlers 👀

Why not CJS entrypoint instead?

// index.mjs

export const IconX = ''
export const IconY = ''
// index.cjs

module.exports = await import('./index.mjs')

This does not work as top level await works only in modules.

Why not ESM only?

Jest does not handle native ESM well, transpilation on consumer side will be required.

spmonahan commented 1 year ago

Another risky attempt might be to ship only CommonJS... ... I am not sure what will be an effect on bundlers 👀

I think this prevents bundlers from tree-shaking code.

layershifter commented 1 year ago

@spmonahan I updated the sandbox. Treeshaking works with Rollup at least, I believe that it could work with Webpack after some tweaks.

spmonahan commented 1 year ago

Cool! I'd love to see how that works.

kkocdko commented 1 year ago

I agree with layershifter's idea about Do not ship ESM, the React itself also do this. But Compress output may caused much longer build time but only reduce a few kb.

MLoughry commented 1 year ago

No, don't unbundle the fonts. The entire purpose of the font work I did was so that Outlook (or any other bundling app concerned about performance) could convert all icons to the font versions, including those in our dependencies (who could be agnostic about SVG vs font).

This was a big performance win for Outlook. Please set up time with us before undoing it.

MLoughry commented 1 year ago

Also, if you're only going to ship one of ESM or CJS, why would you ever pick CJS? It's not statically analyzable, and webpack won't tree-shake it.

layershifter commented 1 year ago

No, don't unbundle the fonts. The entire purpose of the font work I did was so that Outlook (or any other bundling app concerned about performance) could convert all icons to the font versions, including those in our dependencies (who could be agnostic about SVG vs font).

This was a big performance win for Outlook. Please set up time with us before undoing it.

Your work will not be removed, fonts should be just moved to a separate package (for example, @fluentui/react-icons-fonts), so consumers that don't use that feature will not have bloat in their node_modules. Pay-as-you-use.

Also, if you're only going to ship one of ESM or CJS, why would you ever pick CJS?

There is no agreement about that, it's one of proposals. Why pick CJS? Because it works in any environment, ESM code does not have good support in underlying tooling (hello Jest 🙃).

It's not statically analyzable, and webpack won't tree-shake it.

That's not true, Webpack supports tree shaking in CJS, Stackblitz demo

MLoughry commented 1 year ago

Your work will not be removed, fonts should be just moved to a separate package (for example, @fluentui/react-icons-fonts), so consumers that don't use that feature will not have bloat in their node_modules. Pay-as-you-use.

No, I don't think you understand. If 1JS or Fluent or whatever are using the SVG package, and you unbundle the fonts from the package, there's no way for the bundler to coerce those SVG assets to fonts. Currently, that can be done with conditional exports: https://github.com/microsoft/fluentui-system-icons/tree/main/packages/react-icons#using-the-icon-font

// webpack.config.js
module.exports = {
  //...
  resolve: {
    conditionNames: ['fluentIconFont', 'require', 'node'],
  },
};
MLoughry commented 1 year ago

That's not true, Webpack supports tree shaking in CJS, Stackblitz demo

Straight from webpack documentation:

In a 100% ESM module world, identifying side effects is straightforward. However, we aren't there quite yet, so in the mean time it's necessary to provide hints to webpack's compiler on the "pureness" of your code.

ESM is the future; CJS is legacy. Jest support for ESM works fine now.

layershifter commented 1 year ago

Straight from webpack documentation:

In a 100% ESM module world, identifying side effects is straightforward. However, we aren't there quite yet, so in the mean time it's necessary to provide hints to webpack's compiler on the "pureness" of your code.

The quote is irrelevant as it talks about side effects, side effects are not limited to CommonJS.

Webpack 5 adds support for some CommonJs constructs, allows to eliminate unused CommonJs exports and track referenced export names from require() calls.

Check release notes for Webpack 5, https://webpack.js.org/blog/2020-10-10-webpack-5-release/#commonjs-tree-shaking.

ESM is the future; CJS is legacy.

This is opinionated statement and cannot be used as an argument until Node.js will start to emit deprecation warnings on CJS code.

Jest support for ESM works fine now.

image

~Yes~ No

You need to run with a version of node that supports ES Modules in the VM API. See https://jestjs.io/docs/ecmascript-modules

vm.Module is still under flag in Node 20.

https://stackblitz.com/edit/stackblitz-starters-fdrgs9

layershifter commented 1 year ago

No, I don't think you understand. If 1JS or Fluent or whatever are using the SVG package, and you unbundle the fonts from the package, there's no way for the bundler to coerce those SVG assets to fonts. Currently, that can be done with conditional exports: https://github.com/microsoft/fluentui-system-icons/tree/main/packages/react-icons#using-the-icon-font

Sorry, it's still unclear what do you mean. Can you please elaborate?

For example, imports to a package could be replaced by a custom loader:

-import { IconX } from '@fluentui/react-icons'
+import { IconX } from '@fluentui/react-font-icons'

Or even use a Webpack alias.

svdoever commented 1 year ago

Is there a way to solve this issue using Webpack? We don't even use icons (but use @fluentui/react-components) and end up with this mess, almost 14MB to use a few compnents:

547a530c-64a2-49d9-8d95-eaab03a0df18

Making the use use of fluentui a nightmare, because we use it to build widgets for Azure DevOps, and each widget is hosted in its own iframe. Some every widget JavaScript file ends up to be 14MB+ in size. This completely blows up the browser...

layershifter commented 1 year ago

Is there a way to solve this issue using Webpack? We don't even use icons (but use @fluentui/react-components) and end up with this mess, almost 14MB to use a few compnents:

547a530c-64a2-49d9-8d95-eaab03a0df18

Making the use use of fluentui a nightmare, because we use it to build widgets for Azure DevOps, and each widget is hosted in its own iframe. Some every widget JavaScript file ends up to be 14MB+ in size. This completely blows up the browser...

@svdoever This is not related to the problem described this issue. From the screenshot it's visible that you are using CommonJS output (should be ESM) in Webpack that is not expected and indicates a potential problem with your Webpack configuration. Please open a new issue and provide a minimal reproduction there, so we could take a look.

carmezini commented 1 year ago

Is there a way to solve this issue using Webpack? We don't even use icons (but use @fluentui/react-components) and end up with this mess, almost 14MB to use a few compnents: 547a530c-64a2-49d9-8d95-eaab03a0df18 Making the use use of fluentui a nightmare, because we use it to build widgets for Azure DevOps, and each widget is hosted in its own iframe. Some every widget JavaScript file ends up to be 14MB+ in size. This completely blows up the browser...

@svdoever This is not related to the problem described this issue. From the screenshot it's visible that you are using CommonJS output (should be ESM) in Webpack that is not expected and indicates a potential problem with your Webpack configuration. Please open a new issue and provide a minimal reproduction there, so we could take a look.

Hey guys, I am facing the same problem. The new issue was created, after all?

svdoever commented 1 year ago

We moved over to Vite to build our bundles.

Met vriendelijke groet / regards, Serge van den Oever


lainRosensteel commented 8 months ago

This is currently causing a blocking issue when using Fluent UI v9 with the Power Apps Component Framework (PAC). The size of the bundle with only a button and a label is reaching close to 14MB, almost entirely from the icons and fonts. The size is so large that the PAC CLI tools and PCF scripts are unable to upload the code component bundle to Power Apps. In other words, it is currently not possible to use Fluent UI v9 with the PAC CLI tools due to the bundle size being too large. The workaround until this is resolved is to use Fluent UI v8 (or maybe there is some hack with webpack since the PCF scripts use it to bundle).

Technical issues aside, the icons and fonts are covered under a different license. Most of the repository is MIT, except the icons and fonts. This creates a lack of consistency and some legal ambiguity about the use of the icons and fonts as well as the mandatory inclusion of the SVG icons with the bundle. According to the Microsoft Fabric Assets License Agreement, download of the icons and fonts constitutes agreement to the license terms which restricts the use of the icons and fonts:

In connection with the use of a Microsoft API within the development of a software application, website, or product you create or a service you offer designed to provide access or interact with a Microsoft service or application (“Application”)

Although I doubt Microsoft will enforce use of the Segoe UI font for only Microsoft services or applications, it muddies the water.

Having the icons and fonts in a separate repository or package would resolve both the issues; The icon and font coupling issue creating massive bundles which is in turn prevents PAC code components from using Fluent UI v9 without hacks, and the ambiguous licensing issue.

MLoughry commented 8 months ago

The size of the bundle with only a button and a label is reaching close to 14MB, almost entirely from the icons and fonts.

How are you bundling your app? It sounds like you do no tree-shaking at all, if using a single button pulls in the entire package, which is a much bigger issue for you than just this package.

lainRosensteel commented 8 months ago

@MLoughry using the pcf-scripts with a project initialized via pcf init.

There is an official Microsoft guide that uses Fluent UI v8. That all works fine. Once it is setup and tested, migrate from v8 to v9 and you should run into the same issue.

Component Framework: Tutorial Create Canvas Dataset Component

Unfortunately, the official pcf-scripts package is not an open-source project or hosted publicly on GitHub. However, it is viewable on npmjs.com.

Code for the pcf-scripts package

thelok commented 7 months ago

I followed the comment from https://github.com/microsoft/fluentui-system-icons/issues/619#issuecomment-1646525481

I was using "module": "commonjs" in my tsconfig.json.

I changed it to:

"module": "esnext",
"moduleResolution": "node",

and it changed my webpack bundle size from 14MB down to 1.7MB.

lainRosensteel commented 7 months ago

I followed the comment from microsoft/fluentui-system-icons#619 (comment)

I was using "module": "commonjs" in my tsconfig.json.

I changed it to:

"module": "esnext",
"moduleResolution": "node",

and it changed my webpack bundle size from 14MB down to 1.7MB.

That worked! Although ironically topical that the answer was switching from cjs to ESM for the module output. I also tried using "moduleResolution": "node" with cjs (default from the pfc-script tsconfig) and it still generated a 14 MB bundle. So maybe there is still something there in relation to cjs module output with webpack, but the issue I experienced now looks more like a pcf-scripts issue with the init script and their base tsconfig than a fluentui issue. Thanks for looking into that @thelok !

Hotell commented 4 months ago

Jest

Does not seem that icons affect it in any way.

jest/unit-tests are indeed affected https://github.com/microsoft/fluentui/issues/31714#issuecomment-2192050491

Webpack

https://github.com/microsoft/fluentui/issues/24952#issuecomment-2140124554