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

bug: All Angular standalone components are not tree shaken from the main JS chunk #28574

Closed
3 tasks done
StefanNedelchev opened this issue Nov 23, 2023 · 16 comments
Closed
3 tasks done
Labels

Comments

@StefanNedelchev
Copy link

StefanNedelchev commented Nov 23, 2023

Prerequisites

Ionic Framework Version

v7.x

Current Behavior

I tried migrating my current app to Angular 17 and then migrating to Standalone Ionic components. I followed the official guideline and used the automated tool for help. Then I found out that the main JS chunk suddenly became ~1.3MB (compared to the previous ~600kb).

All my modules and pages are lazy loaded and this was a surprise for me. I double checked that all instances of IonicModule are gone and all imports come from the /standalone path. I also double checked that I only import what is needed and where it's needed. Everything seems to be fine. The app is a mixture between modules and standalone components (if this matters).

After checking the bundle that's what I found:

Screenshot from 2023-11-23 21-41-39

As you can see - the main chink is full of Ionic components which are not tree-shaken.

Actually I could also reproduce this in a clean Ionic standalone project with the latest CLI:

Screenshot from 2023-11-23 21-58-55

The issue happens with both webpack and esbuild.

Expected Behavior

I would expect all components to be tree-shaken and bundled in their respective places away from main JS.

Steps to Reproduce

  1. Generate a fresh Ionic project with the latest CLI - Chose Angular and Standalone. For template chose the one with the side menu (because it has an additional lazy loaded module).
  2. Edit the App component by removing all Ionic components from the template, leaving the router outlet only
  3. Remove the imports of these components from the TS file too
  4. Do a production build

Code Reproduction URL

No response

Ionic Info

Ionic:

Ionic CLI : 7.1.5 (/usr/lib/node_modules/@ionic/cli)
Ionic Framework : @ionic/angular 7.5.6
@angular-devkit/build-angular : 17.0.3
@angular-devkit/schematics : 17.0.3
@angular/cli : 17.0.3
@ionic/angular-toolkit : 9.0.0

Capacitor:

Capacitor CLI : 5.5.1
@capacitor/android : not installed
@capacitor/core : 5.5.1
@capacitor/ios : not installed

Utility:

cordova-res : not installed globally
native-run (update available: 2.0.0) : 1.7.4

System:

NodeJS : v18.17.1 (/usr/bin/node)
npm : 9.8.1
OS : Linux 6.2

Additional Information

The app has only Browser platform. It's a mixture between modules and standalone components and all routes are lazy loaded.

@ionitron-bot ionitron-bot bot added the triage label Nov 23, 2023
@StefanNedelchev StefanNedelchev changed the title bug: All Angular standalone components are included in the main JS chunk bug: All Angular standalone components are not tree shaken from the main JS chunk Nov 24, 2023
@hakimio
Copy link

hakimio commented Nov 27, 2023

See @liamdebeasi answer.

@liamdebeasi liamdebeasi self-assigned this Nov 27, 2023
@liamdebeasi
Copy link
Contributor

Thanks for the report. The comment in #28574 (comment) has an explanation for the behavior you are seeing. Ionic is working correctly, but the problem is that the bundle analyzer tool is not showing you the complete picture. Using the tool I noted in the linked comment will give you a more accurate picture of your build size.

@liamdebeasi liamdebeasi closed this as not planned Won't fix, can't repro, duplicate, stale Nov 27, 2023
@liamdebeasi liamdebeasi removed their assignment Nov 27, 2023
@StefanNedelchev
Copy link
Author

@liamdebeasi Thanks for the link, but I'm still confused 🤔 . Why are all components (even if it's just the ones that I use) included in the main chunk? This defies the very purpose of Angular standalone components. Yes, I believe you that this makes the overall size of the bundle smaller but this benefit is overshadowed by the overhead on my main.js. In the example that you show, the overhead is ~50kb (which I would willingly take) but in my case it's an increase from 600kb to almost 1.3 MB which is a deal breaker, especially for my web vitals.

It seems I'll stick to the "old" lazy loaded components for now until something changes. How long do you plan supporting them?

@hakimio
Copy link

hakimio commented Nov 27, 2023

@StefanNedelchev is 1.3MB with esbuild or webpack? In my case, esbuild production bundle is a lot bigger than webpack when using only standalone components and importing everything from @ionic/angular/standalone. There seems to be an issue with esbuild.

@liamdebeasi
Copy link
Contributor

Thanks for the link, but I'm still confused 🤔 . Why are all components (even if it's just the ones that I use) included in the main chunk? This defies the very purpose of Angular standalone components.

Last I checked this behavior is controlled by the Angular CLI/bundler, not Ionic. I'll double check with the Angular team, but as far as I know this is not something Ionic is able to control.

It seems I'll stick to the "old" lazy loaded components for now until something changes. How long do you plan supporting them?

We don't have any immediate plans to remove, but we do plan to slowly move away from IonicModule in favor of standalone components. If the team decides to ultimately remove IonicModule we will give the community ample notice (typically at least 1 year).

There seems to be an issue with esbuild.

Please file a separate issue with a reproduction.

@liamdebeasi
Copy link
Contributor

Last I checked this behavior is controlled by the Angular CLI/bundler, not Ionic. I'll double check with the Angular team, but as far as I know this is not something Ionic is able to control.

I spoke with the Angular team and it sounds like Angular should be doing the desired behavior automatically. I'm working with them to determine why that's not happening right now. I'll follow up here when I have more to share.

@StefanNedelchev
Copy link
Author

@StefanNedelchev is 1.3MB with esbuild or webpack? In my case, esbuild production bundle is a lot bigger than webpack when using only standalone components and importing everything from @ionic/angular/standalone. There seems to be an issue with esbuild.

In my case it's almost similar in both webpack and esbuild. There was a difference but it seemed insignificant to me.

We don't have any immediate plans to remove, but we do plan to slowly move away from IonicModule in favor of standalone components. If the team decides to ultimately remove IonicModule we will give the community ample notice (typically at least 1 year).

Thanks a lot, it will be appreciated. If it's an Angular CLI issue then I hope they will investigate it.

@liamdebeasi
Copy link
Contributor

liamdebeasi commented Nov 30, 2023

The team has been investigating this for the past few days. I've included my findings below.

TL;DR

Both Ionic and Angular/Webpack/ESBuild are working as intended here. Tree shaking is working (meaning unused components are being eliminated from the final build), but code splitting is not ideal due to limitations in bundlers such as Webpack and ESBuild.

Ionic can change how components are imported to avoid this limitation, but it comes with breaking changes and a degraded developer experience. While we're open to making this change in the future, we'd like to have a Request For Comments (RFC) period first and investigate mitigation techniques to preserve the developer experience.

Definitions

For anyone unclear on the difference between tree shaking and code splitting, MDN has some good definitions:

Tree shaking
Code splitting

What behavior is happening?

Ionic components are being included in the first chunk that loads in an Angular application as opposed to being added to the chunks where they are actually used. For example, if main.ts imports provideIonicAngular but home.page.ts imports IonButton, both provideIonicAngular and IonButton are added to the main JS chunk. Developers are expecting provideIonicAngular to be added to the main chunk and IonButton to be added to the chunk for the home page so that IonButton is only loaded when it is actually needed.

Why is this behavior happening?

Ionic ships components from a single entry point (i.e. @ionic/angular/standalone). Some bundlers such as Webpack and ESBuild are not able to split code from a single entry point across chunks. This means that if a single entry point has IonBadge and IonChip, both of those components will always remain in a single chunk.

provideIonicAngular is always included in the main chunk in Ionic Angular, but since that provider is in the same entry point as all the other components, any other component usages also need to be pulled into that main chunk.

Why do we ship components from a single entry point?

This was originally done to make it easy for developers to know where to import components from. In other words, it removes the guess-work associated with figuring out the correct import paths. Our Angular, React, and Vue integrations all have this design.

The alternative is each component is shipped in its own entry point. Developers would need to have 1 import per component which can be cumbersome to manage. We felt it was imported to prioritize developer experience here.

Can bundlers change to account for this behavior?

Yes, but it's not a straightforward process. In fact, ESBuild used to have this behavior but later removed it when top-level await was added: evanw/esbuild#2869. As a result, we don't expect bundlers to support splitting code from a single entry point across chunks anytime soon.

Can Ionic change to account for this behavior?

Yes, but it comes with breaking changes. To optimize for code splitting, each component would need to be in its own entry point. Developer code would need to change like so:

- import { IonHeader, IonToolbar, IonTitle, IonButtons, IonBackButton, IonContent } from '@ionic/angular/standalone';
+ import { IonHeader } from '@ionic/angular/standalone/header';
+ import { IonToolbar } from '@ionic/angular/standalone/toolbar';
+ import { IonTitle } from '@ionic/angular/standalone/title';
+ import { IonButtons } from '@ionic/angular/standalone/buttons';
+ import { IonButton } from '@ionic/angular/standalone/button';
+ import { IonContent } from '@ionic/angular/standalone/content';

Doing this for every single component in an application is time consuming and also makes the code more verbose than before. Other non-component imports such as createAnimation would also need to change.

Is there a path forward to optimize for code splitting while avoiding a degraded developer experience?

I think so, but it's a large amount of work that we do not have bandwidth for at the moment. The original motivation for shipping components from a single entry point still applies, but I think nowadays we have more tools to mitigate the degraded developer experience than we did several years ago. In an ideal world we'd release an RFC that details the following steps we'd take. These steps would need to be taken for Angular, React, and Vue at a minimum:

  1. Ship a feature that utilizes tools such as the Angular Language Service to automatically import components based on usage. This means that when you type ion-content in your HTML, we'd have code that would automatically add the IonContent import in the TypeScript file.
  2. Ship a feature that exports Ionic APIs as separate entry points. Ideally we'd support both this import as well as the current import to avoid immediate breaking changes. However, this may be a significant maintenance burden for the team, so more research would be needed to determine if this is something we are reasonably able to do.
  3. Ship codemods that automatically migrate existing apps from the current import syntax to the new import syntax to ease the migration burden.
  4. Eventually remove the current import syntax in favor of the proposed import syntax.

We could focus on shipping item 2 to save a significant amount of development time, but I think that would be challenging for Ionic developers since they'd effectively have no support in migration/maintenance of this approach.

I can't promise the team will make all of the proposed changes here, but I think it's worth us at least doing an RFC to collect community feedback to start.

@hakimio
Copy link

hakimio commented Nov 30, 2023

Angular Material library is currently using similar code splitting technique where you import MatButtonModule from @angular/material/button, but it's not as fine-grained as proposed Ionic approach where all components are imported from different paths. For example, MatCardModule will contain all the card components. Also, transition from single import path was smooth because of ng update which automatically updated all the code.
Anyway, if you are going this route, there has to be a well documented and "advertised" reliable tool to do the migration automatically. After that I don't think it's such a great idea to leave 3 different paths to import components from. It's already pretty annoying having to choose between @ionic/angular and @ionic/angular/standalone every time when importing Ionic components.

@liamdebeasi
Copy link
Contributor

but it's not as fine-grained as proposed Ionic approach where all components are imported from different paths. For example, MatCardModule will contain all the card components.

To be clear, we aren't set on the approach I detailed above. It was mainly there to give an idea of what a path forward could look like.

It's already pretty annoying having to choose between @ionic/angular and @ionic/angular/standalone every time when importing Ionic components.

If you're using VSCode you can stop it from suggesting @ionic/angular (or vice versa). We have this config in our starter applications you can add: https://github.com/ionic-team/starters/blob/main/angular-standalone/base/.vscode/settings.json

@hakimio
Copy link

hakimio commented Nov 30, 2023

No, WebStorm, but they might have a similar solution. Haven't looked for it.

@hakimio
Copy link

hakimio commented Nov 30, 2023

It does 🙂
image

@cwoebker
Copy link

cwoebker commented Nov 30, 2023

First of all, big thanks for doing such a thorough investigation and solid summary.

Ionic can change how components are imported to avoid this limitation, but it comes with breaking changes and a degraded developer experience.

I can second the comment on the Angular Material library approach here, with automatic imports in most IDEs these days it doesn't really feel like a degraded developer experience. Of course you are right in that this means adapting all imports in a transition which is a bit of effort.

Would be interested to see the RFC and joining the discussion there!

@StefanNedelchev
Copy link
Author

StefanNedelchev commented Dec 1, 2023

@liamdebeasi thank you for not ignoring the "issue" and diving into it. Your comment provides some good insights and also makes me understand why Angular Material imports work the way they do at the moment.
If Ionic is about to make that change and make us provide the additional paths of the import statements for the components, it won't be a problem for me (I hope the majority of the community agrees). Also I believe migrating existing projects to this approach can be automated with a script (but I'm just guessing).
Also something that I wonder - is it possible to actually support both ways of importing if you use a container index.ts file on the /standalone root path that exports everything? I'm not sure if my guess is correct but I think this will make it possible to not break currently imported components but still allow tree shaking if developers start refactoring their imports to be more specific.

@liamdebeasi
Copy link
Contributor

Also I believe migrating existing projects to this approach can be automated with a script (but I'm just guessing).

I haven't tried this, but at first glance this does seem possible.. We'd be migrating the import syntax which I believe is mostly consistent across frameworks (i.e. import { ... } from '@ionic/{angular/standalone, react, vue}'.

is it possible to actually support both ways of importing if you use a container index.ts file on the /standalone root path that exports everything? I'm not sure if my guess is correct but I think this will make it possible to not break currently imported components but still allow tree shaking if developers start refactoring their imports to be more specific.

Yeah that's a good thought. My understanding is the optimized code splitting solution could never export anything from @ionic/angular/standalone (i.e. everything would need to be something like @ionic/angular/standalone/[component]) otherwise we'd be back to the original problem noted in this thread.


Thanks for the feedback, everyone! As noted, our first step here is a public RFC to collect feedback from the community. We'll use that feedback to inform our decisions moving forward.

Copy link

ionitron-bot bot commented Jan 6, 2024

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Jan 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants