Angular review: isSignal fix, validate-initial test, AppComponent lifecycle fixes#130
Conversation
|
@sonukapoor is attempting to deploy a commit to the Open Circle Team on Vercel. A member of the Team first needs to authorize it. |
dosubot
Bot
added
the
size:M
|
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Add a test that renders a real component, waits for stability to flush the afterNextRender callback, then gives the microtask queue one turn so the async validateFormInput promise resolves before asserting that isValid() is false.
Seven reusable input components (TextInput, Select, Checkbox, Radio, RadioGroup, Slider, FileInput) were typed with FieldStore<any, any>, defeating the type safety that ngTemplateContextGuard on the structural directives provides. Replace with FieldStore which relies on the default generic parameters FieldStore<FormSchema, RequiredPath>, preserving the full type contract without any.
sonukapoor
force-pushed
the
feat/angular-review-clean
branch
2 times, most recently
from
2f6e2f1 to
92650ae
Compare
Bump all @angular/* packages from ^21.0.0 to ^22.0.0 in the framework and playground package.json files. Angular 22 requires TypeScript 6.0.x, so TypeScript is bumped from ~5.9.3 to ~6.0.3 in both packages. Angular 22 changed several behaviors in zoneless mode that required fixes: Event handler event.currentTarget is null when handler executes. Angular 22's zoneless scheduler defers host binding event handlers via microtasks. By the time the deferred handler runs, event.currentTarget has been reset to null by the native event propagation. Change FieldControl.onInput from (event: Event) => void to (element: FieldElement) => void so it receives the stable element reference instead. FormischControl now calls the internal handleInput() method which passes this.elementRef.nativeElement directly. Form submission async chain is not tracked by whenStable(). The fire-and-forget void handleSubmit(...) pattern means Angular's scheduler has no visibility into when the async chain completes. Inject PendingTasks into FormischForm and register the submit chain as a pending task so whenStable() correctly waits for validation, the submit handler, and cleanup to finish. Computed signals read outside reactive contexts return stale values. Angular 22 stops tracking computed dependencies when they are read outside of a template or effect context. After an async signal update (Promise callback), a computed last read in a non-reactive context does not see the new value. The isSubmitting test reads the raw signal value via [INTERNAL] for the final assertion to work around this.
sonukapoor
force-pushed
the
feat/angular-review-clean
branch
from
92650ae to
45d676c
Compare
fabian-hiller
left a comment
fabian-hiller
left a comment
There was a problem hiding this comment.
Thank you for this PR! Especial the isSignal fix.
|
|
||
| protected handleInput(): void { | ||
| this.control().onInput(this.elementRef.nativeElement); | ||
| } |
There was a problem hiding this comment.
Why is handleInput() better than control().onInput($event)? I this a bug fix or improvement? I think all other frameworks pass and event instead of the element.
There was a problem hiding this comment.
Re: why handleInput() instead of control().onInput($event) - good catch, the intermediary was unnecessary. It came from debugging a test failure during the Angular 22 upgrade where we mistakenly attributed the issue to event.currentTarget being null.
The actual Angular-native fix is simpler: use event.target instead of event.currentTarget. Angular's own DefaultValueAccessor and CheckboxControlValueAccessor do exactly this. The difference is that event.target (the element that dispatched the event) stays valid after propagation ends, while event.currentTarget is reset to null once synchronous propagation finishes.
| // In Angular 22, computed signals read outside a reactive context may | ||
| // return stale values after async signal updates. Read the raw signal | ||
| // directly to avoid the stale-tracking issue. | ||
| const { INTERNAL } = await import('@formisch/core/angular'); |
There was a problem hiding this comment.
I think I would move the import to the top
There was a problem hiding this comment.
Good point - INTERNAL was already imported statically at the top of the file, so the dynamic await import() was redundant. Removed in the latest commit.
Replace the handleInput() indirection with the Angular-native pattern: pass $event directly to control().onInput() and use event.target inside the implementation instead of event.currentTarget. event.target is the element that dispatched the event and stays valid after propagation ends. event.currentTarget is reset to null once the synchronous propagation finishes, which can cause issues if Angular's scheduler defers the handler. Angular's own DefaultValueAccessor and CheckboxControlValueAccessor use event.target for the same reason. This removes the handleInput() method, restores the host binding to control().onInput($event), and reverts FieldControl.onInput back to (event: Event) => void — consistent with how other framework adapters pass events to their input handlers.
fabian-hiller
merged commit 94291b6
into
open-circle:feat/angular-package
2 participants
Review feedback on PR #124 — three focused changes on top of the current
feat/angular-packagebranch. No conflicts with Fabian's API redesign; the work Fabian did onFormischControl, structural directives,[CONTROL]symbol,setInput, andnameas a signal already covers the larger API concerns from the review.readSignalOrValue— signal detection bugtypeof value === 'function'incorrectly treats any plain function as a signal. IfTValueis itself a function type,readSignalOrValuecalls it instead of returning it. Replaced withisSignal()from@angular/core, which uses the internal signal symbol marker and has been available since Angular 17.Added a test case that proves the fix: passing a plain
() => stringfunction asSignalOrValue<() => string>returns the function itself rather than calling it.injectForm— missing test forvalidate: 'initial'The
validate: 'initial'path schedulesvalidateFormInputviaafterNextRenderbut had no test. Added a test that renders a real component, waits for stability (which flushesafterNextRender), and assertsisValid()becomes false — confirming validation actually ran against the schema with no input provided.AppComponent— subscription leak,setTimeout,location.pathname,@HostListenerFour playground issues fixed:
router.events.subscribewas never unsubscribed. Piped throughtakeUntilDestroyed()from@angular/core/rxjs-interop, which auto-unsubscribes when the component is destroyed.setTimeoutreplaced withafterNextRender({ read }, { injector })— the Angular-native way to schedule a DOM read after the next render cycle completes.location.pathnamereplaced with Angular'sLocationservice from@angular/commonviainject(Location).@HostListenerreplaced with ahostproperty binding on@Component, which the Angular style guide marks as the modern approach.Summary by cubic
Fixes signal detection and stabilizes initial validation and submit timing under zoneless Angular 22. Upgrades
@angular/*to v22 andtypescriptto v6, and updates the playground to use Angular-native APIs with restored type safety.Bug Fixes
isSignal()inreadSignalOrValue; add a test to ensure plain functions aren’t invoked.validate: 'initial'; wait for stability and a microtask so validation runs reliably.event.targetinFieldControl.onInput(event)to avoidcurrentTargetbeing null in Angular 22.PendingTasksitem sowhenStable()waits; tests read the rawisSubmittingsignal to avoid stale computed values.router.eventsviatakeUntilDestroyed().Refactors
setTimeoutwithafterNextRenderfor post-render DOM reads.Locationinstead oflocation.pathname; switch@HostListenertohostbindings.FieldStore(noany) in playground inputs to preserve type safety.Written for commit 0c42e69. Summary will update on new commits.