Repository navigation
Conversation
leonsenft
left a comment
There was a problem hiding this comment.
Nice!
Would you mind splitting the single commit into two: first land the golden, then land the optimization so it's evident from the diff between the first and second commit?
| @@ -572,6 +586,7 @@ export abstract class AbstractControl< | |||
| validators: ValidatorFn | ValidatorFn[] | null, | |||
| asyncValidators: AsyncValidatorFn | AsyncValidatorFn[] | null, | |||
| ) { | |||
| (this as Record<symbol, unknown>)[ABSTRACT_CONTROL] = true; | |||
There was a problem hiding this comment.
Can this just be a field rather than assigned in the constructor?
There was a problem hiding this comment.
Unfortunately not — in ES2022/Rollup, declaring a computed class field (private readonly [ABSTRACT_CONTROL] = true;) evaluates the computed property key during class definition at module evaluation time. Bundlers treat class declarations with computed fields as side-effectful, which prevents AbstractControl (and its related symbols) from being tree-shaken (fails the symbol_test). Keeping the assignment inside the constructor ensures the class declaration remains side-effect-free at the module level.
Add a bundling symbol test and baseline golden file for signal forms applications.
Previously, `FieldNodeContext.valueOf` used `result instanceof AbstractControl` to throw `NG1907` when reading an `AbstractControl` inside a regular `form()`. That value reference prevented `AbstractControl` (along with control event classes and validator helpers) from being tree-shaken in applications that only use signal forms. Introduce an internal symbol-based type guard `ɵisAbstractControl` that checks a private symbol set in `AbstractControl`'s constructor without referencing the `AbstractControl` class value. Fixes angular#71621
5d0d4ef to
3e96a2b
Compare
|
Split into two commits as requested: the first commit introduces the bundling test and baseline golden, and the second commit applies the optimization so the dropped symbols are clearly visible in the diff. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
FieldNodeContext.valueOfchecksresult instanceof AbstractControlto throwNG1907when reading anAbstractControlinside a regularform(). That value reference preventsAbstractControl(along with control event classes and validator helpers) from being tree-shaken in applications that only use signal forms.Issue Number: Fixes #71621
What is the new behavior?
ɵisAbstractControlthat checks a privateSymbolset inAbstractControl's constructor without referencing theAbstractControlclass value, remaining compatible with Closure Compiler (jscompiler) property renaming and ES2022 class tree-shaking.//packages/core/test/bundling/forms_signals:symbol_test) verifying thatAbstractControland its associated classes/helpers are tree-shaken in signal forms bundles.Impact / Dropped Symbols
[formField]apps (//packages/core/test/bundling/forms_signals): Drops 19 symbols (~1.9 KB gzipped) from@angular/forms:AbstractControl,VALID,INVALID,PENDING,DISABLEDValueChangeEvent,StatusChangeEvent,PristineChangeEvent,TouchedChangeEvent,FormSubmittedEventaddValidators,removeValidators,hasValidator,makeValidatorsArray,coerceToValidator,coerceToAsyncValidator,pickValidators,pickAsyncValidators,isOptionsObjform()apps (without[formField]): Drops ~75 symbols (~4.7 KB gzipped), additionally tree-shakingValidators, all built-in validator functions,ControlEvent,FormResetEvent, RxJS async validator helpers (forkJoin,from,innerFrom,scheduled,map, etc.), andtslibasync generator helpers.Does this PR introduce a breaking change?
Other information
Follow-up opportunity for
[formField]FormFieldstill retainsNgControl,AbstractControlDirective,Validators,RequiredValidator,FormResetEvent, andReactiveValidationErrorbecause:FormFieldprovides{provide: NgControl, useFactory: () => inject(FormField).interopNgControl}for CVA /inject(NgControl)interop, andNgControlextendsAbstractControlDirectiveand referencesFormResetEvent,RequiredValidator, andReactiveValidationErrorin its methods.InteropNgControl.hasValidatorandcvaControlCreatereferenceValidators.requiredandValidators.compose.Decoupling the
NgControlDI token from its directive implementation and avoiding direct references to theValidatorsclass inInteropNgControl/cvaControlCreatecould drop an additional ~55 symbols (including the remaining RxJSforkJoin/fromasync validator helpers) from[formField]bundles in a follow-up.