Skip to content

Add reaction() utility#61

Merged
NullVoxPopuli merged 3 commits intoproposal-signals:mainfrom
justinfagnani:reaction
May 13, 2024
Merged

Add reaction() utility#61
NullVoxPopuli merged 3 commits intoproposal-signals:mainfrom
justinfagnani:reaction

Conversation

@justinfagnani
Copy link
Contributor

This adds a reaction() utility that's similar to MobX's reaction() as mentioned in #15 (comment).

reaction() takes two arguments, a data function and an effect function. The data function is wrapped in a Computed, and when the return value of the data function changes, the effect function is called with the current and previous values. An optional equality function is accepted.

const x = new Signal.State(0);
reaction(() => x.get(), (value, previousValue) => {
  console.log('x changed', value, previousValue);
});
x.set(1);
await 0;
// x changed 0 1

The effect function is only called the first time the data function return value changes. It is not called with the initial value.

Reactions can be unsubscribed to by invoking the cleanup function that it returns.

try {
effect(value, previousValue);
} catch (e) {
// TODO: we actually want this to be unhandled, but Vitest complains.
Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli Apr 30, 2024

Choose a reason for hiding this comment

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

how does vitest complain? we can use expect / assert throws:

or for async:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't about asserting that something throws, because the reaction() call isn't on the stack anymore by the time the test effect throws, and doesn't return a promise or something that can contain an async error. This is about user code that throws, but since it's called by a notify callback, letting that be uncaught so that uncaught error handling could handle it.

Without the catch(), Vitest produces an error like:

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Unhandled Errors ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯

Vitest caught 1 unhandled error during the test run.
This might cause false positive tests. Resolve unhandled errors to make sure your tests are not affected.

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Unhandled Rejection ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Error: Oops
 ❯ effect tests/subtle/reaction.test.ts:124:15
    122|       if (value === 1) {
    123|         thrown = true;
    124|         throw new Error("Oops");
       |               ^
    125|       }
    126|       thrown = false;
 ❯ notify src/subtle/reaction.ts:34:9

This error originated in "tests/subtle/reaction.test.ts" test file. It doesn't mean the error was thrown inside the file itself, but while it was running.
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯

Ideally, we exclude just this test from Vitest's unhandled error check. If another test, say a users' own tests, generated an unhandled error, we would want Vitest to fail.

I tried adding an uncaughException handler, but that didn't prevent Vitest's from running.

Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm -- ok, thanks for explaining! I'll take a poke after a readme entry is added -- I want to understand this more 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

I figured something out for handling this, and providing the erroring behavior to users -- lemme know what you think

Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli left a comment

Choose a reason for hiding this comment

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

Lookin good so far! just left a couple notes / comments / suggestions! 🎉

@NesCafe62
Copy link
Contributor

NesCafe62 commented May 3, 2024

hello @justinfagnani
I'm trying to understand the code from this reaction utility. and comparing it with effect implementation.
two questions come to mind:

  1. is it ok that effect uses single global watcher, but reaction creates own instance of watcher on every reaction?
  2. inside reaction effect(value, prevValue) is called synchronously (comparing to effect that uses queueMicrotask). should reaction also use queueMicrotask?

p.s. usually I imagined reaction as an effect with untrack

function reaction(getter, reactionFn) {
   effect(() => {
      const value = getter();
      untrack(() => reactionFn(value));
   });
}

but with hope it is possible for more optimized way (without auto-collect dependency on each call), so watcher is the way to allow that

@justinfagnani
Copy link
Contributor Author

@NesCafe62

is it ok that effect uses single global watcher, but reaction creates own instance of watcher on every reaction?

I guess? effect() is batching all effect callbacks in a single microtask, reaction() will use a separate microtask for each callback. Microtasks are very cheap these days, so I'm not concerned about the cost of them for this use case. I regularly use fast apps that enqueue thousands of microtasks per render.

inside reaction effect(value, prevValue) is called synchronously (comparing to effect that uses queueMicrotask). should reaction also use queueMicrotask?

reaction() is enquing a microtask with this like:

await 0;

Awaiting anything enqueues a microtask. This is similar to await Promise.resolve() or queueMicrotask(...) but without the Promise allocation, or function call + closure allocation. And it's short. I added a comment.

// => 1 logs
```

#### Reactions
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks great!

@NullVoxPopuli NullVoxPopuli merged commit 246e57f into proposal-signals:main May 13, 2024
@NullVoxPopuli NullVoxPopuli added the enhancement New feature or request label May 13, 2024
@github-actions github-actions bot mentioned this pull request May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants