feat: add explicitAfterRenderEffect utility with multi-phase support … - #686
feat: add explicitAfterRenderEffect utility with multi-phase support …#686JonataB wants to merge 2 commits into
Conversation
…and documentation
|
View your CI Pipeline Execution ↗ for commit 2de3af5
☁️ Nx Cloud last updated this comment at |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces explicitAfterRenderEffect, a utility wrapping Angular's afterRenderEffect to provide explicit dependency tracking. It ensures effects only re-run when specified signals change by using untracked for the callback body. The PR includes documentation, unit tests, and support for both single-callback and multi-phase forms. A review comment suggests optimizing the wrapPhase function by using explicit arguments instead of rest parameters to avoid array allocations in performance-critical render phases.
| fn: ( | ||
| values: Input, | ||
| prev: Signal<P> | undefined, | ||
| onCleanup: EffectCleanupRegisterFn, | ||
| ) => R, | ||
| ): ( | ||
| ...args: [EffectCleanupRegisterFn] | [Signal<P>, EffectCleanupRegisterFn] | ||
| ) => R { | ||
| return (...args): R => { | ||
| const values = deps.map((d) => d()) as unknown as Input; | ||
| const [prevOrCleanup, maybeCleanup] = args; | ||
| const onCleanup = (maybeCleanup ?? | ||
| prevOrCleanup) as EffectCleanupRegisterFn; | ||
| const prev = maybeCleanup ? (prevOrCleanup as Signal<P>) : undefined; | ||
|
|
||
| return untracked(() => fn(values, prev, onCleanup)); | ||
| }; | ||
| } |
There was a problem hiding this comment.
The wrapPhase function uses rest parameters (...args), which results in an array allocation on every phase execution. Since afterRenderEffect phases run frequently, it's better to use explicit arguments. Additionally, since wrapPhase is only used for phases that always receive two arguments (write, mixedReadWrite, read), the logic can be simplified to directly accept prev and onCleanup.
fn: (
values: Input,
prev: Signal<P>,
onCleanup: EffectCleanupRegisterFn,
) => R,
): (prev: Signal<P>, onCleanup: EffectCleanupRegisterFn) => R {
return (prev, onCleanup): R => {
const values = deps.map((d) => d()) as unknown as Input;
return untracked(() => fn(values, prev, onCleanup));
};
}
nartc
left a comment
nartc
left a comment
There was a problem hiding this comment.
I'd take gemini suggestion here with a grain of salt. it's not that big of a deal imo but if you feel strongly about it, you can make the adjustment before merging. stamped
Yes. I learn something ❤️ |
Sorry i read only now. We have different signs. Maybe we have to change 'on'. Also ... |
3 participants
…and documentation