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

@fluentui/react-northstar: importing a single component adds >1MB to bundle size #12953

Closed
astegmaier opened this issue Apr 30, 2020 · 28 comments
Assignees
Labels
Fluent UI react-northstar (v0) Work related to Fluent UI V0

Comments

@astegmaier
Copy link
Contributor

astegmaier commented Apr 30, 2020

Environment Information

  • Package version(s): "@fluentui/react-northstar": "0.48.0"
  • Browser and OS versions: All

Please provide a reproduction of the bug in a codepen:

Create a simple app with the following files:
index.tsx

import React from "react";
import ReactDOM from "react-dom";
import App from "./components/App";

ReactDOM.render(<App />, document.getElementById("root"));

components/App.tsx

import React from "react";
import { Provider, themes, Button } from '@fluentui/react-northstar';

const App = () => (
  <Provider theme={themes.teams}>
    <Button content="Hello from FluentUI" />
  </Provider>
);

export default App;

Build it with webpack (or Parcel2) in production mode (which enables tree-shaking and minification), and you'll see a 1.3-1.4MB increase in bundle size over a simple Hello World react app that doesn't use @fluentui/react-northstar.

Here's a visualization of the minified, tree-shaken bundle from webpack, using source-map-explorer:

image

You can see a repro for the issue in the fluentui-button branch of this repository. Run yarn build:webpack (to see webpack bundle output), yarn build:parcel (to see parcel2 bundle output).

This is related to issue #2113, #2324, and #2324 on the legacy repo. Those are still marked as "open", and I don't see any issues or PRs addressing it in the new repo.

Actual behavior:

The bundle size increase of using a single button component is really, really big (1.3-1.4 MB). It looks like it's importing just about every component and icon into the bundle, even though the app uses no icons and only a single button component.

Expected behavior:

The bundle size increase should be "pay-to-play" - the more components/icons that I leverage, the more space it takes, and it should start small.

Priorities and help requested:

Are you willing to submit a PR to fix? Happy to help brainstorm but I don't think I have the time to fashion a full PR.

Requested priority: Medium (before a v1 release, would think it's important to get this under control).

@mnajdova
Copy link
Contributor

mnajdova commented May 4, 2020

@astegmaier that for creating the issue. We already started working on cutting the bundle size. The icons are already separated in their own package, which reduced the app bundle size for about 34%, you can see more info on that on this blog post. We started with removing some component statics that were referenced from the theme, but there are still some problems. We are doing an investigation around this now, so we are going to create a plan and share it by the end of next week.

@astegmaier
Copy link
Contributor Author

astegmaier commented May 4, 2020

Thanks for the details! In the original issue, I was seeing the all the icons in the bundle (i.e. the optimization you mentioned in the blog post) wasn't being reflected in either parcel or webpack.

I managed to tweak webpack to do better tree-shaking by using babel-loader instead of ts-loader, or by keeping ts-loader and making sure to set "module": "es6" in my tsconfig.json. Parcel was still including all the icons though (increasing bundle size by ~0.5 MB), and I opened this issue to see if the problem might be with Parcel instead of fluentui.

@mnajdova
Copy link
Contributor

mnajdova commented May 5, 2020

Currently we are tracking the bundle size only for webpack. Let me take a look on the issue on the parcel repo and will get back to you tomorrow. Maybe it would be good to add bundle size testsfor more bundlers.

@astegmaier
Copy link
Contributor Author

astegmaier commented May 5, 2020

It looks like parcel2 currently doesn't tree-shake re-exports quite as well as webpack - if you comment out line 301 from @fluentui/react-northstar/src/index.ts...

    //export * from '@fluentui/react-icons-northstar';

...the bundle size of my sample app in parcel2 shrinks from 1.1MB to 777KiB (compared to a roughly consistant 660 KiB in webpack). Hopefully, the parcel team will be able to address this before the beta release (its tracked in #4565).

Having webpack as the main standard for now seems fine to me, but since the JavaScript world changes quickly, and tree-shaking/scope-hoisting is more of an art than a science, you might get better results if you considered multiple bundlers from the outset. I'm an occasional contributor to parcel as well, and we could always use the feedback 😄

Also, you might want to look at this comment for some ideas on how make bundle size even better going forward.

@mnajdova
Copy link
Contributor

mnajdova commented May 7, 2020

Thanks for the input @astegmaier and the links to the comments. I cloned your repo and I am waiting on the new release to test again it, as we removed static className from the components so at least those are not referenced from the theme. I hope this will enable us to not have all components in the bundle when only one is used (there may be some other statics that we haven't resolved yet, but I will have to test this).

I wouldn't remove the re-export for the icons from @fluentui/react-northstar, because is convenient for users to be able to pick up the icons from the same package. If this will be solved soon, than we should not have problems with this.

We are removing slowly the withSafeTypeForAs by moving our components to be functional components, so that one will be resolved soon too. 👍

Will update you once I try the new release and will post any findings here.

@mnajdova
Copy link
Contributor

@astegmaier update from my side. I upgraded locally to v0.49 andI hoped we will see better results, but it was similar. Fortunately I found the issue. In each file we are defining constants for the className of the component and it's slots, and we are exporting these in the following manner:

export * from 'component-file-path';

Just moving these contants in a dedicated file is solving the issue surprisingly.. This worked and was tested with webpack, on both your repo (fluentui-button-babel branch) and with our local bundle test tools.

This looks really strange for me, but will do some final reading and testing tomorrow, and if I don't find better reason of why this is happening, I will start with creating the files for these classes for all components, which will solve the problem with dragging all components when importing the theme.

@astegmaier
Copy link
Contributor Author

@mnajdova That's great news! Thanks for investigating.

@mnajdova
Copy link
Contributor

Here is the PR for reference: #13145 I am still surprised by this to be honest, so will ask other people for review and opinion too.

@layershifter
Copy link
Member

layershifter commented May 26, 2020

Just moving these contants in a dedicated file is solving the issue surprisingly.. This worked and was tested with webpack, on both your repo (fluentui-button-babel branch) and with our local bundle test tools.

Just to clarify: the issue with remaining components in bundles is related to side-effects that we have. One of them are static properties: https://dev.to/layershifter/how-to-kill-tree-shaking-in-webpack-with-static-properties-p72


I went through all cases to summarize required actions:

@latonita
Copy link

@layershifter @mnajdova would you please give us users status update on this issue. Thank you.

@layershifter
Copy link
Member

@latonita the work that was done in #13985 & #14007 decreased bundle size significantly. Now it should be around 450kb on minimal component and theme imports (we started with 1.2mb). Still not good, but it's definitely better.

image

The next huge item is a theme object which is not treeshakable 😨 However, this requires huge changes in the current design of Northstar components.

@latonita
Copy link

latonita commented Sep 15, 2020

@layershifter thanks for update!

any ideas how can I reduce icons package at least? 465Kb for the pack, but i only use 5 icons.

image

@layershifter
Copy link
Member

@latonita it should not be a case if you're using 0.51.0 or later. Result below is for two icons, they are completely treeshakable:

image

image

I created a sample repo to check: https://github.com/layershifter/sample-fluentui

@syedjahangirpeeran
Copy link

syedjahangirpeeran commented Oct 15, 2020

@layershifter I'm also facing the same issue, a single component imported from @fluentui/react-northstar is taking the bundle size to 1.2mb. I'm using the latest 0.51.2 version.

@layershifter
Copy link
Member

layershifter commented Oct 15, 2020

@syedjahangirpeeran can you please share a minimal repro repository? For example you can fork https://github.com/layershifter/sample-fluentui.

@latonita
Copy link

@layershifter just checked - i'm on 0.49. i will upgrade to 51+ today and let you know

@zeroliu
Copy link

zeroliu commented Oct 20, 2020

@layershifter Slightly related to this issue. The giant bundle size is causing webpack to take seconds to rebuild.

You can start any new webpack project with the default development config. When I only include an index.ts with a console.log. It takes 500ms to build and generates a file of 577byte. Once I import anything from @fluentui/react-northstar, the build binary increases to 4.25mb and it takes over 7 seconds to rebuild.

This slow iteration problem also happens to all projects generated with https://github.com/pnp/generator-teams. I think it's a very bad experience for a ms recommended teams bootstrap project to take 7sec to rebuild on every single change.

@latonita
Copy link

latonita commented Oct 26, 2020

@layershifter i finally made it to 0.51.2 today, had to fix some theme (updateTheme stopped working, btw, had to write my own updateTheme) and some grid changes.
However nothing is changed in package sizes.
I removed package lock and node_modules; npm i.
I still have 541 Kb of react-icons-northstar/dist/es components
I'm only using several icons, i'm importing them as
import { AddIcon, CloseIcon, ErrorIcon } from "@fluentui/react-icons-northstar";

any suggestions? shall I import differently?
p.s. its teams app, originally generated by yeoman teams-generator

@layershifter
Copy link
Member

any suggestions? shall I import differently?
p.s. its teams app, originally generated by yeoman teams-generator

@latonita can you please create and share a small repo that I can run to check?

@latonita
Copy link

@layershifter okay. here it is https://github.com/latonita/test-teams-fluentui
I have just generated new app. nothing extra is added. fluentui was upgraded from 49 to 51.2
App is not using any icons. However it uses themes to match user-selected theme.
Straight from the beginning it has all icons included. 541kb.
stats.json attached.

@layershifter
Copy link
Member

layershifter commented Oct 26, 2020

@latonita Thanks for sharing, that's something 👍 There is an issue with ModuleConcatenation that prevents tree-shaking:

    /*! ModuleConcatenation bailout: Cannot concat with ./node_modules/@fluentui/accessibility/dist/es/attributes.js because of ./node_modules/@fluentui/react-northstar/dist/es/index.js */
    /*! ModuleConcatenation bailout: Cannot concat with ./node_modules/@fluentui/accessibility/dist/es/behaviors/Accordion/accordionBehavior.js because of ./node_modules/@fluentui/react-northstar/dist/es/index.js */
    /*! ModuleConcatenation bailout: Cannot concat with ./node_modules/@fluentui/accessibility/dist/es/behaviors/Accordion/accordionContentBehavior.js because of ./node_modules/@fluentui/react-northstar/dist/es/index.js */
    /*! ModuleConcatenation bailout: Cannot concat with ./node_modules/@fluentui/accessibility/dist/es/behaviors/Accordion/accordionTitleBehavior.js because of ./node_modules/@fluentui/react-northstar/dist/es/index.js */
    /*! ModuleConcatenation bailout: Cannot concat with ./node_modules/@fluentui/accessibility/dist/es/behaviors/Alert/alertBaseBehavior.js because of ./node_modules/@fluentui/react-northstar/dist/es/index.js */

It's not related to Fluent UI itself, it's related to third party dependency called msteams-react-base-component as it does not support ES Modules:

image

😿

A line with require('@fluentui/react-northstar') causes the explosion 💣


It also visible by public tools:

image
https://www.skypack.dev/view/msteams-react-base-component


Once I manually complied it to ESM I got 652KB vs 1.4 MiB previously:

image

@latonita
Copy link

latonita commented Oct 27, 2020

hm.. i see its a part of teams generator.
https://github.com/wictorwilen/msteams-react-base-component

@layershifter any recomendation how to fix? config changes? source code fixes? meanwhile i will try to remove that package

To be honest, i dont see anything very bad there. Small class, here is the header with import statements

// Copyright (c) Wictor Wilén. All rights reserved.
// Licensed under the MIT license.

import * as React from "react";
import { render } from "react-dom";
import { themes } from "@fluentui/react-northstar";
import { ThemePrepared } from "@fluentui/styles";
import * as microsoftTeams from "@microsoft/teams-js";

I have just removed that package - just copied that class to the app. Again, nothing changed :)
I pushed updates and stats.json to my repo https://github.com/latonita/test-teams-fluentui

@layershifter
Copy link
Member

@latonita oops, I missed the second important change:

tsconfig-client.json

{
    "compilerOptions": {
-        "module": "commonjs",
+        "module": "esnext"

@layershifter
Copy link
Member

@latonita I created two PRs and described them to explain the issue:

wictorwilen/msteams-react-base-component#5
pnp/generator-teams#156

@layershifter any recomendation how to fix? config changes? source code fixes? meanwhile i will try to remove that package

Please check wictorwilen/msteams-react-base-component#5, there are no issues with source itself there is an issue missing ESM dist.

@latonita
Copy link

latonita commented Oct 27, 2020

@layershifter i think thats only part of the issue.
i updated tsconfig-*.
i removed lib, added the only file from there
build it.
now all the icons/theme moved to under that file TeamsBaseComponent.tsx.
i do believe the problem is in
import { teamsTheme, teamsDarkTheme, teamsHighContrastTheme } from "@fluentui/react-northstar";
which is mandatory when we would like to support different themes. and this lead to loading all the resources, even those which are not used....

image

@layershifter
Copy link
Member

@latonita I pulled your latest master and update tsconfig-client.json, it's what I have:

image

There is no icons included.


We can't do anything with red area now: you're correct all themes are big objects that are not tree-shakable, so will you will anyway pay for themes if they are included. However, actual components will not be included to the bundle, webpack-bundle-analyzer output is not accurate:

image

This can be verified by using my Webpack's config for debugging: https://github.com/layershifter/sample-fluentui/blob/a5ffea15b696f0c0d2df1d694d20bfab1418cc3d/webpack.config.js#L14-L38. With it you can build partially minified bundle and ensure that components are not included even if webpack-bundle-analyzer says that they will. It happens that because webpack-bundle-analyzer relies on Webpack's output, but dead-code elimination is done later by Terser.

@latonita
Copy link

@layershifter thanks for the help! 👍

@layershifter
Copy link
Member

As there is no work planned or any possible improvements - closing this for housekeeping 🏡

@microsoft microsoft locked as resolved and limited conversation to collaborators Dec 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Fluent UI react-northstar (v0) Work related to Fluent UI V0
Projects
None yet
Development

No branches or pull requests

10 participants