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: [7.5] standalone build size is bigger than standard version #28358

Closed
3 tasks done
hatsantos opened this issue Oct 15, 2023 · 14 comments · Fixed by #28359
Closed
3 tasks done

bug: [7.5] standalone build size is bigger than standard version #28358

hatsantos opened this issue Oct 15, 2023 · 14 comments · Fixed by #28359
Labels
bug: external Bugs in non-Ionic software that impact Ionic apps (I.e. WebKit, Angular, Android etc) package: angular @ionic/angular package

Comments

@hatsantos
Copy link

hatsantos commented Oct 15, 2023

Prerequisites

Ionic Framework Version

v7.x

Current Behavior

When converted to a standalone components app, using Ionic standalone components, the app build size is not smaller as announced in the blog post.

Using a sample Ionic app from the CLI, such as "sidemenu," after converting it to standalone components (following the guide: https://ionicframework.com/docs/angular/build-options#migrating-from-modules-to-standalone), the initial app size is larger than the module version.

Ionic "sidemenu" sample app and updated to Ionic 7.5 and Ionicons 7.2.1

ionic build --prod

image

Ionic "sidemenu" sample app updated to Ionic 7.5 and Ionicons 7.2.1 and migrated to standalone components

ionic build --prod

image

Am i doing something wrong?

The migration effort is still high, and in doing so, I would like to ensure that it's worth it and that I'm doing it the right way.

Expected Behavior

Initial bundle size to be smaller.

Steps to Reproduce

  1. ionic start > angular > standalone > sidemenu
  2. build a prod app
  3. update to standalone version of ionic components
  4. build a prod app

Code Reproduction URL

https://[email protected]/hatslab/ionstandalone.git

Ionic Info

Ionic:

Ionic CLI : 7.1.1 (C:\Users\username\AppData\Local\pnpm\global\5.pnpm@[email protected]\node_modules@ionic\cli)
Ionic Framework : @ionic/angular 7.5.0
@angular-devkit/build-angular : 16.2.6
@angular-devkit/schematics : 16.2.6
@angular/cli : 16.2.6
@ionic/angular-toolkit : 9.0.0

Capacitor:

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

Utility:

cordova-res : not installed globally
native-run : 1.7.3

System:

NodeJS : v20.8.1 (C:\Program Files\nodejs\node.exe)
npm : 10.1.0
OS : Windows 10

Additional Information

Code Reproduction URL has a standalone ionic component converted app.

@ionitron-bot ionitron-bot bot added the triage label Oct 15, 2023
@hatsantos hatsantos changed the title bug: standalone build size is bigger than standard version bug: [7.5] standalone build size is bigger than standard version Oct 15, 2023
@gant02
Copy link

gant02 commented Oct 16, 2023

I have the exact same behavior with my app following the migration guide. All standalone components are imported in the main.js bundle. With the tab starter, everything is imported in a dedicated chunk but all at once.
Capture d’écran 2023-10-16 à 10 53 17

@liamdebeasi liamdebeasi self-assigned this Oct 16, 2023
@liamdebeasi
Copy link
Contributor

Thanks for the report. I can reproduce a larger than expected bundle size. It looks like certain generated component wrappers have side effects which are causing them to be pulled into the build. In particular, we manually generate a handful of components to apply extra customizations. These manually generated wrappers reassigned, creating the side effect.

You'll notice that some auto generated components are in there too. For example, IonItem (auto generated) is being pulled in because it is used inside of IonDatetime (manually generated).

I'm still digging into this, so there may be other issues too but this is what I know so far.

@liamdebeasi
Copy link
Contributor

liamdebeasi commented Oct 16, 2023

It looks like this was introduced in 28f2ec9. Ionic wasn't creating side effects, but the way ng-packagr generated the components caused a side effect. I was able to avoid the ng-packagr issue.

Here's a dev build with a proposed fix if you are interested in testing: 7.5.1-dev.11697480817.10fa2601

@liamdebeasi liamdebeasi added package: angular @ionic/angular package type: bug a confirmed bug report labels Oct 16, 2023
@ionitron-bot ionitron-bot bot removed the triage label Oct 16, 2023
@liamdebeasi liamdebeasi added triage bug: external Bugs in non-Ionic software that impact Ionic apps (I.e. WebKit, Angular, Android etc) and removed type: bug a confirmed bug report labels Oct 16, 2023
@ionitron-bot ionitron-bot bot removed the triage label Oct 16, 2023
@hatsantos
Copy link
Author

@liamdebeasi Thank you, I'll test it and provide feedback as soon as possible.

@liamdebeasi
Copy link
Contributor

liamdebeasi commented Oct 16, 2023

Thanks! I found a bug in the dev build I posted, so make sure you grab the latest dev build in the edited comment above.

@hatsantos
Copy link
Author

hatsantos commented Oct 16, 2023

I ran some tests. The raw size seems to have reduced by half and the gzip size improved; however, compared to the initial NgModule version, the size is still similar (slightly larger even). I thought it would be possible to achieve better results.

Prod 7.5.0 build sidemenu sample app, NgModule version

image

Prod 7.5.0 build, sidemenu sample app converted to standalone components:

image

image

Dev 7.5.1-dev.11697480817.10fa2601 build, using sidemenu sample app converted to standalone components:

image

image

@liamdebeasi
Copy link
Contributor

liamdebeasi commented Oct 16, 2023

Your NgModule test does not account for the "Lazy Chunk Files" which will significantly change the results of your test. If you look at the size of www, you'll get (roughly) the following:

NgModules on 7.5.0: 7.2mb
Standalone on 7.5.0: 1.1mb
Standalone on 7.5.1-dev.11697480817.10fa2601: 631kb

The NgModule version of Ionic lazily loads files over the network as needed whereas the Standalone version does not which is why the initial chunk sizes are a bit different.

@liamdebeasi liamdebeasi removed their assignment Oct 16, 2023
@gant02
Copy link

gant02 commented Oct 17, 2023

On my side, the main.js bundle is still bigger than before. All ionic components used in the app (even in other pages) are included in the main.js.

I generated a new project with side menu and we have:

image

But if you add in the folder components for example, the components is added in the main.js.

image

It should be in a specific chunk no? it was the case in version prior 7.5.1.

With a large application, the main.js is bigger with v7.5.1 than before.

@liamdebeasi
Copy link
Contributor

@gant02 Please provide a reproduction case so I can test.

@gant02
Copy link

gant02 commented Oct 18, 2023

Hello @liamdebeasi ,

code reproduction is available here: https://github.com/gant02/test_standalone.git

Ion-textarea is added in the main bundle instead of dedicated chunk although it's in a specific standalone component.

@liamdebeasi
Copy link
Contributor

liamdebeasi commented Oct 18, 2023

This isn't an Ionic bug. The Angular CLI opted to put this into the main.js chunk by default. There may be options in the Angular CLI to change which chunk this is added to, but Ionic doesn't control this.

This thread is dedicated to identifying Ionic components that are not being used in an application but are still being included in the bundle. Textarea is included in the bundle because you are using it in your application.

@gant02
Copy link

gant02 commented Oct 18, 2023

Oh ok so i misunderstood the benefits of this feature, i thought that it would remove unnecessary components but keep the lazy loading feature/chunks. Project size is lower but main.js is bigger.

If a component is needed, angular CLI automatically add it in the main.js (and i don't see how to correct that) which is bad for a web app (but ok for a mobile app).
With the IonicModule, angular CLI create specific chunk for each components so it's lazy loaded when needed.

So clearly this feature should be avoided for a web app. Maybe you can add this drawback in your docs? it would avoid me this migration if i knew it before.

But thanks for the explanation, i will stop looking for a solution and revert my code.

github-merge-queue bot pushed a commit that referenced this issue Oct 18, 2023
Issue number: resolves #28358

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->


28f2ec9
exposed a (possible) `ng-packagr` bug where the form control components
were being re-assigned, which breaks treeshaking. These components were
considered side effects and were always being pulled into the bundle.

This resulted in a higher than expected bundle size. This issue appears
to be caused by using 2 decorators **and** referring to the class in
`useExisting` (for providers). Doing just one of these does not
reproduce the issue.

The compiled output looks something like this:

```typescript
let IonToggle = IonToggle_1 = /*@__PURE__*/ class IonToggle extends ValueAccessor {
    constructor(c, r, z, injector) {
        super(injector, r);
        this.z = z;
        c.detach();
        this.el = r.nativeElement;
        proxyOutputs(this, this.el, ['ionChange', 'ionFocus', 'ionBlur']);
    }
    writeValue(value) {
        this.elementRef.nativeElement.checked = this.lastValue = value;
        setIonicClasses(this.elementRef);
    }
    handleIonChange(el) {
        this.handleValueChange(el, el.checked);
    }
};
/** @nocollapse */ IonToggle.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "14.2.12", ngImport: i0, type: IonToggle, deps: [{ token: i0.ChangeDetectorRef }, { token: i0.ElementRef }, { token: i0.NgZone }, { token: i0.Injector }], target: i0.ɵɵFactoryTarget.Component });
/** @nocollapse */ IonToggle.ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "14.0.0", version: "14.2.12", type: IonToggle, isStandalone: true, selector: "ion-toggle", inputs: { checked: "checked", color: "color", disabled: "disabled", enableOnOffLabels: "enableOnOffLabels", justify: "justify", labelPlacement: "labelPlacement", legacy: "legacy", mode: "mode", name: "name", value: "value" }, host: { listeners: { "ionChange": "handleIonChange($event.target)" } }, providers: [
        {
            provide: NG_VALUE_ACCESSOR,
            useExisting: IonToggle_1,
            multi: true,
        },
    ], usesInheritance: true, ngImport: i0, template: '<ng-content></ng-content>', isInline: true, changeDetection: i0.ChangeDetectionStrategy.OnPush });
IonToggle = IonToggle_1 = __decorate([
    ProxyCmp({
        defineCustomElementFn: defineCustomElement$1i,
        inputs: TOGGLE_INPUTS,
    })
], IonToggle);
```

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- Removed the `ProxyCmp` usage in favor of manually calling proxyInputs
and proxyMethods.
-  Also saw that select was missing a form control test, so I added one

The compiled code now looks something like this:

```typescript
class IonToggle extends ValueAccessor {
    constructor(c, r, z, injector) {
        super(injector, r);
        this.z = z;
        defineCustomElement$1i();
        proxyInputs(IonToggle, TOGGLE_INPUTS);
        c.detach();
        this.el = r.nativeElement;
        proxyOutputs(this, this.el, ['ionChange', 'ionFocus', 'ionBlur']);
    }
    writeValue(value) {
        this.elementRef.nativeElement.checked = this.lastValue = value;
        setIonicClasses(this.elementRef);
    }
    handleIonChange(el) {
        this.handleValueChange(el, el.checked);
    }
}
/** @nocollapse */ IonToggle.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "14.2.12", ngImport: i0, type: IonToggle, deps: [{ token: i0.ChangeDetectorRef }, { token: i0.ElementRef }, { token: i0.NgZone }, { token: i0.Injector }], target: i0.ɵɵFactoryTarget.Component });
/** @nocollapse */ IonToggle.ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "14.0.0", version: "14.2.12", type: IonToggle, isStandalone: true, selector: "ion-toggle", inputs: { checked: "checked", color: "color", disabled: "disabled", enableOnOffLabels: "enableOnOffLabels", justify: "justify", labelPlacement: "labelPlacement", legacy: "legacy", mode: "mode", name: "name", value: "value" }, host: { listeners: { "ionChange": "handleIonChange($event.target)" } }, providers: [
        {
            provide: NG_VALUE_ACCESSOR,
            useExisting: IonToggle,
            multi: true,
        },
    ], usesInheritance: true, ngImport: i0, template: '<ng-content></ng-content>', isInline: true, changeDetection: i0.ChangeDetectionStrategy.OnPush });
```

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

Ryan provided some context on a related Stencil bug where doing
reassignments broke treeshaking in Webpack. While the source of this bug
is not Stencil, understanding the Stencil bug helped me better
understand this issue:

ionic-team/stencil#3191
ionic-team/stencil#3248
ionic-team/stencil#4188 (fixes an issue
introduced in the above stencil PR)

Dev build: `7.5.1-dev.11697480817.10fa2601`
@liamdebeasi
Copy link
Contributor

Oh ok so i misunderstood the benefits of this feature, i thought that it would remove unnecessary components but keep the lazy loading feature/chunks. Project size is lower but main.js is bigger.

Thanks for the clarification with your project. We don't plan to note how the main.js chunk will change because that will vary per-app based on a) your Angular configuration, b) how many components you are using, and c) which bundler you are using (Webpack or ESBuild). Stating that the main.js chunk will always grow or shrink in size may be misleading.


That being said, the fix for the original bug has been merged in #28359. This fix will be available in an upcoming release of Ionic. Thanks again!

sean-perkins pushed a commit that referenced this issue Oct 27, 2023
Issue number: resolves #28358

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->


28f2ec9
exposed a (possible) `ng-packagr` bug where the form control components
were being re-assigned, which breaks treeshaking. These components were
considered side effects and were always being pulled into the bundle.

This resulted in a higher than expected bundle size. This issue appears
to be caused by using 2 decorators **and** referring to the class in
`useExisting` (for providers). Doing just one of these does not
reproduce the issue.

The compiled output looks something like this:

```typescript
let IonToggle = IonToggle_1 = /*@__PURE__*/ class IonToggle extends ValueAccessor {
    constructor(c, r, z, injector) {
        super(injector, r);
        this.z = z;
        c.detach();
        this.el = r.nativeElement;
        proxyOutputs(this, this.el, ['ionChange', 'ionFocus', 'ionBlur']);
    }
    writeValue(value) {
        this.elementRef.nativeElement.checked = this.lastValue = value;
        setIonicClasses(this.elementRef);
    }
    handleIonChange(el) {
        this.handleValueChange(el, el.checked);
    }
};
/** @nocollapse */ IonToggle.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "14.2.12", ngImport: i0, type: IonToggle, deps: [{ token: i0.ChangeDetectorRef }, { token: i0.ElementRef }, { token: i0.NgZone }, { token: i0.Injector }], target: i0.ɵɵFactoryTarget.Component });
/** @nocollapse */ IonToggle.ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "14.0.0", version: "14.2.12", type: IonToggle, isStandalone: true, selector: "ion-toggle", inputs: { checked: "checked", color: "color", disabled: "disabled", enableOnOffLabels: "enableOnOffLabels", justify: "justify", labelPlacement: "labelPlacement", legacy: "legacy", mode: "mode", name: "name", value: "value" }, host: { listeners: { "ionChange": "handleIonChange($event.target)" } }, providers: [
        {
            provide: NG_VALUE_ACCESSOR,
            useExisting: IonToggle_1,
            multi: true,
        },
    ], usesInheritance: true, ngImport: i0, template: '<ng-content></ng-content>', isInline: true, changeDetection: i0.ChangeDetectionStrategy.OnPush });
IonToggle = IonToggle_1 = __decorate([
    ProxyCmp({
        defineCustomElementFn: defineCustomElement$1i,
        inputs: TOGGLE_INPUTS,
    })
], IonToggle);
```

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- Removed the `ProxyCmp` usage in favor of manually calling proxyInputs
and proxyMethods.
-  Also saw that select was missing a form control test, so I added one

The compiled code now looks something like this:

```typescript
class IonToggle extends ValueAccessor {
    constructor(c, r, z, injector) {
        super(injector, r);
        this.z = z;
        defineCustomElement$1i();
        proxyInputs(IonToggle, TOGGLE_INPUTS);
        c.detach();
        this.el = r.nativeElement;
        proxyOutputs(this, this.el, ['ionChange', 'ionFocus', 'ionBlur']);
    }
    writeValue(value) {
        this.elementRef.nativeElement.checked = this.lastValue = value;
        setIonicClasses(this.elementRef);
    }
    handleIonChange(el) {
        this.handleValueChange(el, el.checked);
    }
}
/** @nocollapse */ IonToggle.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "14.2.12", ngImport: i0, type: IonToggle, deps: [{ token: i0.ChangeDetectorRef }, { token: i0.ElementRef }, { token: i0.NgZone }, { token: i0.Injector }], target: i0.ɵɵFactoryTarget.Component });
/** @nocollapse */ IonToggle.ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "14.0.0", version: "14.2.12", type: IonToggle, isStandalone: true, selector: "ion-toggle", inputs: { checked: "checked", color: "color", disabled: "disabled", enableOnOffLabels: "enableOnOffLabels", justify: "justify", labelPlacement: "labelPlacement", legacy: "legacy", mode: "mode", name: "name", value: "value" }, host: { listeners: { "ionChange": "handleIonChange($event.target)" } }, providers: [
        {
            provide: NG_VALUE_ACCESSOR,
            useExisting: IonToggle,
            multi: true,
        },
    ], usesInheritance: true, ngImport: i0, template: '<ng-content></ng-content>', isInline: true, changeDetection: i0.ChangeDetectionStrategy.OnPush });
```

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

Ryan provided some context on a related Stencil bug where doing
reassignments broke treeshaking in Webpack. While the source of this bug
is not Stencil, understanding the Stencil bug helped me better
understand this issue:

ionic-team/stencil#3191
ionic-team/stencil#3248
ionic-team/stencil#4188 (fixes an issue
introduced in the above stencil PR)

Dev build: `7.5.1-dev.11697480817.10fa2601`
github-merge-queue bot pushed a commit that referenced this issue Oct 31, 2023
Issue number: resolves #28431

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->

My previous attempt at fixing
#28358 caused inputs
to no longer be correctly proxied to the underlying components. This was
an attempt to work around an underlying ng-packagr bug (see linked
thread for more info).

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

I decided it would be best to continue using `ProxyCmp` (since we know
that works) and find an alternative to working around the ng-packagr
bug. I spoke with the Angular team, and they recommended pulling the
provider into its own object. `forwardRef` is now required since we are
referencing the component before it is declared.

- Revert
82d6309
- Moves provider to an object to avoid ng-packagr issue
- I reverted the proxy e2e tests. These are no longer needed since we
are not ejecting from the typical `ProxyCmp` usage anymore.

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->
Dev build: `7.5.3-dev.11698699090.1151d73f`
Verified that the issue is fixed with the repro provided in
#28431

Also verified that this does not regress the issue described in
#28358.
Copy link

ionitron-bot bot commented Nov 17, 2023

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 Nov 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug: external Bugs in non-Ionic software that impact Ionic apps (I.e. WebKit, Angular, Android etc) package: angular @ionic/angular package
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants