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

Ideas for better tree-shaking of the Forms package #41011

Closed
AndrewKushnir opened this issue Feb 26, 2021 · 5 comments
Closed

Ideas for better tree-shaking of the Forms package #41011

AndrewKushnir opened this issue Feb 26, 2021 · 5 comments
Assignees
Labels
Milestone

Comments

@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Feb 26, 2021

While doing investigation in a context of #40997, I came across a few areas in Forms module that can be refactored to provide better tree-shaking.

IMPORTANT: I'm creating this ticket primarily to capture some ideas/proposals. The numbers that are used in the ticket are for a particular use-case only, the real result might be different based on a particular scenario.


The use-case that I've looked at was an <input> with an ngModel on it: <input type="text" [(ngModel)="title">.

Currently (with Angular 11.2.2) the Forms package size for such use-case is 41.81kb. I've built an app using command from #40997 (comment), so it's not a fully optimized version (with all optimizations it's 30.11kb). While doing the investigation I prototyped some of the changes to get a sense of potential savings we can get from improved tree-shaking.

  1. [PR perf(forms): make built-in ControlValueAccessors more tree-shakable #41146] Built-in Control Value Accessors are not tree-shakable due to this code:

const BUILTIN_ACCESSORS = [
CheckboxControlValueAccessor,
RangeValueAccessor,
NumberValueAccessor,
SelectControlValueAccessor,
SelectMultipleControlValueAccessor,
RadioControlValueAccessor,
];
export function isBuiltInAccessor(valueAccessor: ControlValueAccessor): boolean {
return BUILTIN_ACCESSORS.some(a => valueAccessor.constructor === a);
}

This can be refactored to be tree-shakable by adding a special field onto these classes (for ex ɵbuiltinCVA) and checking if this field exists in the isBuiltInAccessor function instead of referring to these classes directly.

Performing this refactoring allows to drop 10.12kb (~25% reduction), so the Forms package reduced to 31.69kb for that use-case.

  1. [PR perf(forms): avoid direct references to the Validators class #41189] Extract some logic from the Validators class and use them directly in the Forms code.

Currently the Validators class contains some helper functions (such as Validators.compose, Validators.composeAsync and Validators.nullValidator) that are used across the code of the Forms package. We should consider extracting these functions, using them directly in the code (to avoid references to Validators class) and use these helpers in the Validators class too (to avoid any breaking changes).

This optimization allows to save 3.36kb (~8% reduction), so the Forms package reduced to 28.33kb for that use-case.

  1. Avoid direct references to FormArray and FormGroup in the _find function.

The mentioned _find function is used in the AbstractControl, so once the FormControl is present in the code (which is used under the hood in the NgModel directive), the FormArray and FormGroup become non-tree-shakable as well (even if they are not used anywhere else). Possible refactoring it to do something similar to what I mentioned in the 1st section (regarding Built-in CVAs).

That can give another 7kb of savings (or 16.7% reduction), so the Forms package size is 21.3kb.

  1. [PR perf(forms): make FormBuilder and RadioControlRegistry tree-shakable #41126] I also noticed that the RadioControlRegistry class is non-tree-shakable (as it's directly referenced in the FormsModule and ReactiveFormsModule). We can consider making it a tree-shakable provider (by using providedIn: FormsModule), but that might potentially be a breaking change.

Making RadioControlRegistry tree-shakable saves a bit less than 1kb, so the final Forms package size in my experiment was 20.5kb, which is half the size of the original package (41.81kb).

Note: in real scenarios savings would be much less than in my example, since most of the Forms code would actually be used (such as validators, FormGroup and FormArray, different types of inputs -> more CVAs), but improved tree-shaking should still provide benefits.

@AndrewKushnir
Copy link
Contributor Author

AndrewKushnir commented Feb 27, 2021

  1. [PR perf(forms): make FormBuilder and RadioControlRegistry tree-shakable #41126] In addition to making the RadioControlRegistry provider tree-shakable (as mentioned above), we should also consider making FormBuilder tree-shakable as well, i.e.:
@Injectable({providedIn: ReactiveFormsModule})
export class FormBuilder { ... }

Instead of referencing it directly here:

providers: [FormBuilder, RadioControlRegistry],

@itea-dev
Copy link
Contributor

Improving tree shaking and reducing the bundle size of default blank app generated by CLI will allow more users to take advantage of Angular.

At present we have some embedded apps built with Vue because we can't fit in Angular. And I would like in the near future to use Angular for all our apps and be able to use the same libraries, styles, project structure and coding standards we already have and use in our current Angular apps.

@AndrewKushnir
Copy link
Contributor Author

Just want to mention a couple more things that might contribute to the payload size reduction (not necessarily related to tree-shaking), mostly micro-optimizations though (i.e. likely without massive size reductions), but still worth looking at.

  1. Consider moving repeated logic from CVAs to a new base classes.

Currently all CVAs implement the setDisabledState function:

setDisabledState(isDisabled: boolean): void {
this._renderer.setProperty(this._elementRef.nativeElement, 'disabled', isDisabled);
}

which is in fact duplicated across all of them. We should consider creating a base class (for ex. AbstractControlValueAccessor, analogous to AbstractControl) which would contain base/common logic, such as setDisabledState.Similarly, the registerOnTouched function looks duplicated in CVAs too (thus can be moved to the AbstractControlValueAccessor class):

registerOnTouched(fn: () => {}): void {
this.onTouched = fn;
}

One more function that can be implemented there is setProperty helper method, which would shorten the calls like:

this._renderer.setProperty(this._elementRef.nativeElement, 'disabled', isDisabled);

to just:

this.setProperty('disabled', isDisabled);

Eventually, we can consider making AbstractControlValueAccessor a part of the public API, so that developers can leverage this class in their custom CVAs (and avoid duplication of certain logic).

Important note: the AbstractControlValueAccessor should be designed as a strongly-typed one (so we don't have to revisit that in the future while working on type improvements for other parts of Forms).

  1. Make the code a bit more compact in AbstractControlDirective class.

The AbstractControlDirective class delegates calls to underlying control, such as (an in many other cases):

get valid(): boolean|null {
return this.control ? this.control.valid : null;
}

We can refactor it by creating a tiny function to call this.control and use it in all places, similar to #40651.


There might be more places where we can do refactoring to reduce payload size (without sacrificing code maintainability), I will keep this thread updated and keep adding more info.

Also adding a reference to issue #39640 that I created earlier, which talks about AbstractControl-based classes in a context of tree-shaking of util methods.

@AndrewKushnir
Copy link
Contributor Author

Update: items 1 (Built-in CVAs), 2 (Validators class), 4 (RadioControlRegistry provider) and 5 (FormBuilder provider) from the list above were implemented and are likely to be included into the next patch release (v11.2.6).

I've also investigated the Avoid direct references to FormArray and FormGroup in the _find function. item, but it turned out that in most situations the FormGroup class is needed even in the Template-driven-based apps, since it's used in the NgForm directive, which is initialized for most of the <form> elements (with some exceptions, see directive selector).

We could still look into making the code a bit more compact as described in items 6 (Base class for CVAs, draft PR #41225) and 7 (AbstractControlDirective refactoring), but it'd not provide any major improvements from code size perspective.

I also did a quick test just to confirm that the above-mentioned optimizations work correctly and for a simple app (that just has one input element <input type="text" [(ngModel)]="count">) the @angular/forms package size (built in prod mode) reduced from 30.12kb to 21.73Kb, which is 8.39kb or ~28% reduction. Important note: in real applications where more code from the @angular/forms package is imported/referenced, the saving would actually be less, but some unused code should still be tree-shaken away.

Due to the fact that Forms logic heavily uses classes, achieving better tree-shaking without major architectural changes is not feasible at this moment (see #39640 for additional info).

I'm closing this ticket since most of the work outlined here is completed.

Thank you.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 17, 2021
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

2 participants