Angular review: signal safety, native directive, type guards, lifecycle fixes#129
Closed
sonukapoor wants to merge 7 commits into
Closed
Angular review: signal safety, native directive, type guards, lifecycle fixes#129sonukapoor wants to merge 7 commits into
sonukapoor wants to merge 7 commits into
Conversation
The props object and ref callback were React concepts with no equivalent in Angular templates. Remove FieldElementProps and the props sub-object from FieldStore. Move name, autofocus, onFocus, onChange, and onBlur directly onto FieldStore so Angular developers access field state without a props namespace wrapper. Remove the ref callback. Angular templates have no mechanism to call a ref callback on a native element, so the internal elements array was never populated and focus management silently did nothing. Add registerElement(element): () => void to FieldStore as the replacement registration API. It pushes the element into the internal store and returns a per-element cleanup function, replacing the coarse isConnected filter that ran on component destroy. Add FormischFieldElementDirective ([formischFieldElement]). This is the Angular-native integration point, equivalent to [formControl] or Angular CDK directives like [cdkDrag]. Applied to any focusable element inside a formisch-field template, it: - Sets [attr.name] automatically via a host binding - Delegates (focus), (input), (change), and (blur) DOM events to field.onFocus, field.onChange, and field.onBlur via host bindings, covering both text inputs (input event) and checkboxes/selects (change event) without any manual template wiring - Registers the element via registerElement in the afterNextRender write phase and cleans up precisely via DestroyRef when destroyed With the directive a bare input only needs value bound explicitly: <input [formischFieldElement]="field" [value]="field.input()" /> For custom wrapper components the flat API is used directly: [name]="field.name" (fieldFocus)="field.onFocus($event)" (fieldChange)="field.onChange($event)" (fieldBlur)="field.onBlur($event)" Update all playground pages, tests, and type tests to use the flat API.
Two issues existed in the previous implementation: The inner <form> element was located via querySelector on the host, which is fragile and bypasses Angular's view query system. Replace this with a viewChild.required reference using a #formEl template variable so the reference is typed, direct, and managed by Angular. Class forwarding ran only once inside afterNextRender. If a parent bound classes dynamically to <formisch-form> after the initial render, those changes were never picked up. Replace the class-forwarding portion with afterRenderEffect using the earlyRead/write phase split: - earlyRead reads the host element's className from the DOM without touching it, returning it as the phase result. - write receives that result as a Signal<string>, reads it, and if non-empty applies it to the inner <form> element and clears the host attribute. Using a dedicated write phase prevents layout thrashing by keeping DOM reads and writes in separate phases. afterRenderEffect runs after every render cycle, so class changes applied by Angular's change detection are always forwarded. Once the host class is cleared the write phase becomes a no-op on subsequent renders unless the parent re-binds a class. Element registration (writing the form element reference into the internal store) remains in afterNextRender with a write phase since the element reference never changes after the first render. Capture hostEl, formEl, and ofSignal as local constants before the callbacks to avoid this references inside effect closures, consistent with the pattern used in FormischFieldElementDirective. Add a dedicated FormischForm class forwarding describe block to the test suite that verifies classes are moved from the host to the inner form element and removed from the host after render.
Without a type guard, the let-field and let-fieldArray variables in ng-template blocks are typed as any by Angular's template type checker. This means users get no autocompletion on field.input(), field.errors(), fieldArray.items(), or any other FieldStore and FieldArrayStore members. Add a static ngTemplateContextGuard method to both components. Angular's template type checker calls this method to narrow the $implicit context type when a consumer writes let-field or let-fieldArray on an ng-template. The guard propagates the component's TSchema and TFieldPath/TFieldArrayPath generics into the template variable type, so the IDE can provide accurate completions and type errors for the full FieldStore and FieldArrayStore surfaces. Export FormischFieldContext and FormischFieldArrayContext interfaces alongside the components so consumers can reference the context type directly if needed (e.g. when manually typing a TemplateRef).
Three Angular-nativeness issues existed in the root app component.
The router.events subscription was never unsubscribed, leaking memory
whenever the component is destroyed (e.g. in tests or SSR). Pipe it
through takeUntilDestroyed() from @angular/core/rxjs-interop, which
automatically unsubscribes when the component's DestroyRef fires. Also
add a filter() operator to narrow the event type to NavigationEnd so the
subscribe callback is typed and the instanceof check is gone.
setTimeout was used to defer the DOM read after navigation so that
Angular could finish updating routerLinkActive state before reading
offsetLeft and offsetWidth. Replace it with afterNextRender({ read },
{ injector }) which is the Angular-native equivalent: it schedules the
callback in the read phase after Angular has completed its next render
cycle, using the component's injector so the ref is scoped correctly.
location.pathname referenced the global browser Location object directly,
which would throw in SSR and bypasses Angular's routing abstractions.
Replace it with Angular's Location service from @angular/common injected
via inject(Location), using this.location.path() to get the current path.
Also replace the @HostListener decorator on onResize with a host property
binding on the @component decorator. The Angular style guide marks
@HostListener as legacy and recommends the host object for new code.
Add FormischFieldElementDirective.test.ts covering the four host bindings: name attribute is set from the field path, focus event marks the field as touched, input event updates the field value, and change event also updates the field value. All other exported APIs have runtime tests; this fills the gap for the new directive. Switch the validate-initial path in injectForm from the plain afterNextRender callback to the phased write form, consistent with every other afterNextRender call in the package.
|
@sonukapoor is attempting to deploy a commit to the Open Circle Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dosubot
Bot
added
size:XL
Contributor
Author
|
Closing — the upstream branch has been significantly updated since this was opened. Will rebase and re-evaluate which changes are still relevant. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR applies the Angular-nativeness review requested by Fabian on PR #124. All changes are on top of the existing
feat/angular-packagebranch with no modifications to any other code.Seven commits covering the issues found during the review.
readSignalOrValue— signal detection bugtypeof value === 'function'incorrectly identifies any function as a signal. Replaced withisSignal()from@angular/core, which uses Angular's internal signal symbol marker and is available since Angular 17.propsobject andrefcallback — React patterns removedThe
propssub-object (field.props.name,field.props.onFocus, etc.) and therefcallback are React concepts with no equivalent in Angular templates. Removed both.name,autofocus,onFocus,onChange, andonBlurare now flat onFieldStoredirectly. Therefcallback is replaced byregisterElement(element): () => voidwhich returns a precise per-element cleanup function.Added
FormischFieldElementDirective([formischFieldElement]), the Angular-native integration point equivalent to[formControl]or Angular CDK directives. Applied to any focusable element inside aformisch-fieldtemplate, it sets[attr.name]and wires(focus),(input),(change), and(blur)via host bindings — no manual event wiring needed:For custom wrapper components the flat API is used directly without a
props.prefix.validate: 'initial'— missing testAdded a test that renders a real component, triggers the render cycle, and asserts that
isValid()becomes false after initial validation runs viaafterNextRender.FormischFormclass forwarding — runs once, usesquerySelectorReplaced
querySelectorwithviewChild.requiredfor a typed direct reference to the inner<form>. Split the work into two lifecycle hooks:afterNextRenderwithwritephase for the one-time element registration, andafterRenderEffectwithearlyRead/writephases for class forwarding so it re-runs reactively after every render cycle. Added a test that verifies classes are moved from the host to the inner form element.ngTemplateContextGuard— typedlet-fieldcontextWithout a guard,
let-fieldandlet-fieldArrayinng-templateblocks are typed asany. Addedstatic ngTemplateContextGuardto bothFormischFieldandFormischFieldArraywith exportedFormischFieldContextandFormischFieldArrayContextinterfaces. Users now get full IDE autocompletion onfield.input(),field.errors(),fieldArray.items(), etc.AppComponent— subscription leak,setTimeout,location.pathname,@HostListenerrouter.events.subscribewas never unsubscribed. Piped throughtakeUntilDestroyed()from@angular/core/rxjs-interop.setTimeoutreplaced withafterNextRender({ read }, { injector })— the Angular-native way to schedule a DOM read after the next render cycle.location.pathnamereplaced with Angular'sLocationservice from@angular/common.@HostListenerreplaced withhostproperty binding on@Component(the Angular style guide marks@HostListeneras legacy).Summary by cubic
Makes the Angular package feel native and type-safe. Replaces React-style field props/ref with a flat API, adds the
[formischFieldElement]directive, fixes signal detection, and improves<formisch-form>lifecycle behavior.Migration
field.props.name→field.name,field.props.autofocus→field.autofocus.field.props.onFocus/onChange/onBlur→field.onFocus/onChange/onBlur.[formischFieldElement]on native inputs for name + event wiring and element registration, e.g.<input [formischFieldElement]="field" [value]="field.input()" />.nameand events from the flat API; if needed programmatically, callfield.registerElement(el)and use the returned cleanup.FormischFieldElementDirective(re-exported from the Angular entrypoint) where used.Bug Fixes
isSignalfrom@angular/coreinreadSignalOrValueto prevent treating plain functions as signals.validate: 'initial'using phasedafterNextRender.<formisch-form>now forwards host classes to the inner<form>on every render viaafterRenderEffect, and registers the form withviewChild.required.setTimeoutwithafterNextRender, useLocationfrom@angular/common, and move@HostListenertohostbindings.Written for commit 381e1be. Summary will update on new commits.