preactjs / signals

Manage state with style in every framework
https://preactjs.com/blog/introducing-signals/
MIT License
3.83k stars 95 forks source link

Regression: unexpected non-computed batch behavior #145

Closed WebReflection closed 2 years ago

WebReflection commented 2 years ago

I have been testing preact/signals-core together with usignal and solid-js but after installing latest my tests, which run just fine with latter two libraries and used to run just fine before latest release, now fail with preact.

The test in case contains this code:

const {assert} = console;

(function testBatchMore() {
  const invokes = [];
  const counter = signal(0);
  const double = computed(() => counter.value * 2);
  const tripple = computed(() => counter.value * 3);

  effect(() => {
    invokes.push([double.value, tripple.value]);
  });

  assert(invokes.length === 1, 'effect not working in bached more');

  batch(() => {
    counter.value = 1;
    // here double.value is actually 0 instead!
    assert(double.value === 2, 'computed side-effecting within batch');
  });

  assert(invokes.length === 2, 'unexpected more batched result');
  assert(invokes[0].join(',') === '0,0', 'unexpected batched more values');
  assert(invokes[1].join(',') === '2,3');
})();

If you'd like to test yourself:

# git clone ... usignal
cd usignal
npm i
cd test
npm i
cd ..
npm test preact
# see difference with
# npm test solid
# npm test usignal

If this regression was actually an intentional change, it would be great to understand how come preact signals decided to behave differently from solid-js there (I am just the last one playing around with signals here, I don't have strong opinion about anything šŸ˜Š)

Thanks.

WebReflection commented 2 years ago

P.S. the reason I believe this is a regression is that here the example explicitly states the intent:

import { signal, computed, effect, batch } from "@preact/signals-core";

const counter = signal(0);
const double = computed(() => counter.value * 2);
const tripple = computed(() => counter.value * 3);

effect(() => console.log(double.value, tripple.value));

batch(() => {
    counter.value = 1;
    // Logs: 2, despite being inside batch, but `tripple`
    // will only update once the callback is complete
    console.log(double.value);
});
// Now we reached the end of the batch and call the effect

FWIWI I've based 100% code coverage on most of your examples (and more) so, thanks folks, this project has been both inspiring and fun as R&D side-project.

marvinhagemeister commented 2 years ago

Oh good catch! Agree, with you that doesn't look right and is not intentional on our part. Maybe an area where our test suite is not exhaustive enough yet. Checked the history and the regression was introduced in #127.

Thanks for reporting this issue and happy to hear that you enjoy playing around with the code šŸ‘

WebReflection commented 2 years ago

feel free to copy/paste my tests which do 100% code coverage for usignal and might help having code coverage in here too (I haven't seen that running in PRs) ... you want to look at test/preact.js and test/test.js (but if you want to use test/leak.js too go ahead, as I am planning to keep testing usignal, preact, and solid there (at least) šŸ‘‹