Leveraging `reaction()` with dependencies in the effect.

[Westbrook]

I'm looking at using a reaction like this: https://codepen.io/Westbrook/pen/QwNJdBK?editors=0010

import { Signal } from 'https://unpkg.com/signal-polyfill?module';
import { SignalSet } from 'https://unpkg.com/signal-utils@0.21.1/dist/set.ts.js?module';
import { reaction } from 'https://unpkg.com/signal-utils@0.21.1/dist/subtle/reaction.ts.js?module';

const trackedSet = new SignalSet();
const trackedValue = new Signal.State(123);
let changes = 0;

document.body.textContent += `
Initial: ${JSON.stringify({
             setValues: [...trackedSet],
             stringValue: trackedValue.get()
         })}

`;

reaction(
    () => {
         const value = trackedValue.get();
         return {
             value,
             setValues: [...trackedSet],
         };
    },
    (current, previous) => {
      console.log('reaction');
        document.body.textContent += `
Current: ${JSON.stringify(current)}
Previous: ${JSON.stringify(previous)}

`;
        console.log({ current });
        console.log({ previous });
        changes += 1;
        trackedSet.add(changes);
    }
);

queueMicrotask(() => {
  trackedValue.set(1234);
  trackedSet.add(0);
  queueMicrotask(() => {
    trackedValue.set(12345);
    trackedSet.add(-1);
  });
});

Notice the the second set of changes do not queue a reaction:

  queueMicrotask(() => {
    trackedValue.set(12345);
    trackedSet.add(-1);
  });

If you comment out the dependency change in the effect, then the subsequent changes trigger the effect.

        // trackedSet.add(changes);

I can understand not reacting to the change in dependency durring the effect, but I'm confused what's causing the subsequents updates not to be reacted to.

Possible correction

I'm currently seeing two possible solutions to this.

Watch in effect
The first option is the watch in the effect. The path that I see to make this possible is to call the next watch before running the effect:

            let notify: (() => Promise<void>) | undefined = async () => {
		// await 0 is a cheap way to queue a microtask
		await 0;
		// Check if this reaction was unsubscribed
		if (notify === undefined) {
			return;
		}
		const value = computed.get();
+              // Restart the watch before the effect() in case dependencies change there.
+		watcher.watch();
		if (!equals(value, previousValue)) {
			try {
				effect(value, previousValue);
			} catch (e) {
				// TODO: we actually want this to be unhandled, but Vitest complains.
				// We probably don't want to enable dangerouslyIgnoreUnhandledErrors
				// for all tests
				if (__internal_testing__) {
					console.error(e);
					__internal_testing__.lastError = e;
				} else {
					throw new ReactionError(e);
				}
			} finally {
				previousValue = value;
			}
		}
-		watcher.watch();
	};

This prevents the reactions from being blocked down the line, BUT it also explicitly will begin listening to changes in the effect(). This may not be expected or desired.

Flush before watch
I see in effect()

signal-utils/src/subtle/microtask-effect.ts

Lines 8 to 16 in 1b9b4be

	let watcher = new Signal.subtle.Watcher(() => {
	if (!pending) {
	pending = true;
	queueMicrotask(() => {
	pending = false;
	flushPending();
	});
	}
	});

that there is a auto-flush mechanism after the effect is run. Should similar be inluded within the reaction() workflow?

	let notify: (() => Promise<void>) | undefined = async () => {
		// await 0 is a cheap way to queue a microtask
		await 0;
		// Check if this reaction was unsubscribed
		if (notify === undefined) {
			return;
		}
		const value = computed.get();
		if (!equals(value, previousValue)) {
			try {
				effect(value, previousValue);
			} catch (e) {
				// TODO: we actually want this to be unhandled, but Vitest complains.
				// We probably don't want to enable dangerouslyIgnoreUnhandledErrors
				// for all tests
				if (__internal_testing__) {
					console.error(e);
					__internal_testing__.lastError = e;
				} else {
					throw new ReactionError(e);
				}
			} finally {
				previousValue = value;
			}
		}
+		// Flush pending effects
+		for (const signal of watcher.getPending()) {
+			signal.get();
+		}
		watcher.watch();
	};

There's likely other, better solutions to this issue, but it seems unexpected that changes in this part of the reaction() workflow would pause reactions for the lifecycle of the Signal, so hopefully there might be some clarity on which path would be best here. I'm happy to covert either of this, or the better third option, into a PR to get these changes included in the project as desired.

CC: @justinfagnani as you wrote the initial implementation of this.

EDITS:

• move watcher.watch() after const value = computed.get(); in first option.