Skip to content

feat(reactive-on): extend usages of the reactive on utility - #688

Open
JeanMeche wants to merge 1 commit into
ngxtension:mainfrom
JeanMeche:on-extended
Open

feat(reactive-on): extend usages of the reactive on utility#688
JeanMeche wants to merge 1 commit into
ngxtension:mainfrom
JeanMeche:on-extended

Conversation

@JeanMeche

Copy link
Copy Markdown
Contributor

You can now use it with computer & afterRenderEffect

@nx-cloud

nx-cloud Bot commented May 4, 2026

Copy link
Copy Markdown

View your CI Pipeline Execution ↗ for commit bd62c38

Command Status Duration Result
nx affected --target=build --parallel=3 --exclu... ✅ Succeeded <1s View ↗
nx affected --target=test --parallel=3 --exclud... ✅ Succeeded 4s View ↗
nx affected --target=lint --parallel=3 ✅ Succeeded 40s View ↗
nx-cloud record -- nx format:check ✅ Succeeded 1s View ↗

☁️ Nx Cloud last updated this comment at 2026-05-24 18:04:07 UTC

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the on utility to support computed and afterRenderEffect by returning computation results and making the cleanup function optional. It also adds relevant tests. Feedback focuses on restoring JSDoc documentation and maintaining type safety by using unknown instead of any for internal variables.

Comment on lines +7 to 11
/**
* Makes dependencies of a computation explicit.
*
* // is equivalent to:
* effect(() => {
* const v = a();
* untracked(() => console.log(v, b()));
* });
* ```
* Works with `effect`, `computed`, and `afterRenderEffect`.
*/

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The previous JSDoc included a helpful example and an explanation of how on works (by using untracked internally). Removing these details makes the utility harder to understand for new users. It would be beneficial to keep the explanation and update the example to include the new supported use cases like computed.

/**
 * Makes dependencies of a computation explicit.
 *
 * Works with `effect`, `computed`, and `afterRenderEffect`.
 *
 * @example
 * ```typescript
 * effect(on(a, (v) => console.log(v, b())));
 *
 * // is equivalent to:
 * effect(() => {
 *   const v = a();
 *   untracked(() => console.log(v, b()));
 * });
 *
 * // also works with computed:
 * const doubled = computed(on(count, (val) => val * 2));
 * ```
 */

Comment thread libs/ngxtension/reactive-on/src/on.ts Outdated
Comment on lines +80 to +81
let prevInput: any;
let prevValue: any;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Changing these variables from unknown to any reduces type safety within the implementation. Since fn is called with these values and its implementation signature accepts any, unknown is perfectly compatible and safer. It's recommended to stick with unknown as it was before.

	let prevInput: unknown;
	let prevValue: unknown;

Comment thread libs/ngxtension/reactive-on/src/on.ts Outdated
return (onCleanup: EffectCleanupRegisterFn) => {
let input: unknown;
return (onCleanup?: EffectCleanupRegisterFn) => {
let input: any;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using any for the input variable is less safe than unknown. Since input is assigned from various sources and then passed to fn, unknown is a better fit for internal type safety.

		let input: unknown;

@JeanMeche
JeanMeche force-pushed the on-extended branch 2 times, most recently from 73666c7 to 53f7062 Compare May 4, 2026 15:08
nartc
nartc previously approved these changes May 6, 2026

@nartc nartc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gemini suggestion seems legit. JSDoc comment with examples is helpful.

@nartc
nartc dismissed their stale review May 6, 2026 18:07

didn't realize it was still draft

@JeanMeche
JeanMeche force-pushed the on-extended branch 2 times, most recently from bd62c38 to ada6a4c Compare May 24, 2026 18:00
You can now use it with `computer` & `afterRenderEffect`
const log: number[] = [];

effect(
on({ a, b }, ({ a: valA, b: valB }) => {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking more about it, I don't think we should implicitly invoke the signals.

@JeanMeche
JeanMeche marked this pull request as ready for review May 27, 2026 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants