microsoftgraph / microsoft-graph-toolkit

Authentication Providers and UI components for Microsoft Graph 🦒
https://docs.microsoft.com/graph/toolkit/overview
Other
936 stars 296 forks source link

[BUG]Conflicted with vite, bundle size too big #1457

Closed sep2 closed 10 months ago

sep2 commented 2 years ago

Describe the bug

When use it with vite, the following output in console:

@microsoft/mgt-msal2-provider is incorrectly packaged. Please contact the package author to fix. @microsoft/mgt-react is incorrectly packaged. Please contact the package author to fix.

The final bundle is too big (452kb uncompressed) that it includes all the components in the output bundle even only import a single one <Person />.

To Reproduce Use vite default config to build app, import <Person /> component and render it.

Expected behavior No console error in vite, and the output should be tree-shaked to contain only the imported components.

Environment (please complete the following information):

Additional context The @azure/msal-browser is also a big bundle (233kb uncompressed), bug not as big as this library.

ghost commented 2 years ago

Hello sep2, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌

sebastienlevert commented 2 years ago

Can you share a repo that can reproduce this issue or a bit more code so we can look into this? I'm wondering if this comes from the way mgt-react exports the web components. Adding @nmetulev to this conversation for more details!

sep2 commented 2 years ago

Here is repo: https://github.com/sep2/mgt-vite

sebastienlevert commented 2 years ago

Thanks for sharing the sample! I was able to reproduce and it's an area we'd love to improve in the future. For now, we will be adding for our Future milestone and see how this gets prioritized, thanks!

NathZ1 commented 1 year ago

Hi @sebastienlevert, i can second this - I am analysing the bundle size of a small production React application that uses mgt-login and msal2 provider for user authentication. It doesn't use any other MGT component and obviously doesn't have a need to use any other provider.

mgt-components is taking up 25% of the entire build just for the single login component!

You can also see the size of @azure as well for msal-browser and msal-common packages which the providers use. Annoyingly, the old mgt-msal provider (I realise this has been removed in 3.0) is also causing the msal package to be included (another 100kb).

All in all, it's pretty inefficient to use mgt in this way, but I think it would be very useful if we could pick and choose what we are using without the worry about build sizes due to unused components etc.

image

NathZ1 commented 1 year ago

just an update to this - i couldn't really justify close to 1MB for a single login component (and associated auth), so I had 2 options, write my own component and auth using msal-browser, or manually mod the packages so webpack ignored the parts I didn't need. I ended up going for option 2 and got it working with just a couple of edits:

1) @microsoft/mgt/dist/es6/index.js & index.d.ts - commented out the exports for all the unused providers - image

2) @microsoft/mgt-components/dist/es6/components/components.js & components.d.ts - commented out the exports for all the unused components - image

rebuilding the app resulted in a large size reduction which was well worth the effort - 2.4MB --> 1.7MB node_modules (pre-gzip) - image

I'm going to lock the mgt packages to the current installed version, and use patch-package to apply the changes post-install so i can apply across dev machines easily.

Would certainly welcome a better solution that allowed webpack to tree shake mgt packages properly :)

Cheers

sebastienlevert commented 1 year ago

Can you help us with how we should think about this issue? What would be your preferred approach for this?

NathZ1 commented 1 year ago

Sorry, I don't really have any experience in building packages :(

It looks like there are a few blogs online but when searching for 'create packages that allow tree shaking', which might be helpful?

eg. https://blog.theodo.com/2021/04/library-tree-shaking/

sebastienlevert commented 1 year ago

@gavinbarron as discussed today for tree shaking / bundling all or not all components.

gavinbarron commented 1 year ago

Dang. Well this needs to be prioritized and fixed soon IMO.

@NathZ1 thanks so much for raising this, I was of the mistaken opinion that the unused component would be eliminated via tree shaking.

gavinbarron commented 1 year ago

So, quick note on a rapid hack test.

None of our packages are marked as being side effect free, so tree shaking cannot eliminate the unused code. Using the supplied repro I did a couple of tests just to look at the bundling, with no functional testing.

I ran the analyze tool in the supplied repro to look at the vendor bundled file size with and without hacking in sideEffects=false into the package.json file for the mgt libraries

current state sideEffects=false
mgt 2.3.1 911.76 KiB 401.21 KiB
mgt 3.0.1 1444.76 KiB 537.65 KiB

Now this change needs some through testing to ensure we don't break existing application and will only be made to the v3.x as we're not performing non-critical maintenance on the 2.x version. But initial experiments show great promise.

gavinbarron commented 1 year ago

In testing with simply marking our libraries as side effect free I found that actually all our code gets eliminated as importing @microsoft/mgt-components is a gigantic side effect. In retrospect I should have realized this as we use a decorator on the component classes to define them in the customElements registry and one of the reasons why it's necessary to use a dynamic import if you're using disambiguation in v3.

Anyhow, I've thrown together a potential approach eliminate most of our side effects, enable tree shaking, and not create a breaking change for folk importing from the root of the mgt-components library.

Now this is definitely not perfect, based on the bundle inclusions I was able to see using rollup to test bundling when using mgt-components that it looks like too much from fast and fluent ui were being included, but at least now we have some improvement.

It's up as a draft PR in #2642 and on npm under the next.enable-tree-shaking tag