nank1ro / solidart

Signals in Dart and Flutter, inspired by SolidJS
https://docs.page/nank1ro/solidart
MIT License
155 stars 5 forks source link

[chore] Correct `Computed`'s `hasPreviousValue` getter #43

Closed manuel-plavsic closed 1 year ago

manuel-plavsic commented 1 year ago

Currently, the returned value is always true, since the first time a value is set, hasPreviousValue is also set to true.

The PR ensures that hasPreviousValue returns true starting from the second time a value is — non-manually — set, i.e., the first time a value is updated.

In the first commit (fee2a4b270148b134fb281178411e0164ca1ccc8) can cause unnecessary rebuilds. The second commit (c3be4db) fixes this issue.

codecov[bot] commented 1 year ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (dev@2f64d0d). Click here to learn what that means. The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##             dev      #43   +/-   ##
======================================
  Coverage       ?   97.48%           
======================================
  Files          ?       18           
  Lines          ?      716           
  Branches       ?        0           
======================================
  Hits           ?      698           
  Misses         ?       18           
  Partials       ?        0           
nank1ro commented 1 year ago

That's wrong. The first value of computed is set here https://github.com/nank1ro/solidart/pull/43/files#diff-ff1336af3256cbb826fb12484c57514aeec782f3a3fc8c6a0f8d4a77a5572fe0L73

Please provide some code that shows the issue so I can easily understand how isn't working as expected.

manuel-plavsic commented 1 year ago

You are right. I was a bit unsure because the initial value was always true. I gave an example in the other PR.

The issue has something to do with SignalBuilder (see https://github.com/nank1ro/solidart/pull/42#issuecomment-1598945357).

manuel-plavsic commented 1 year ago

_SignalBuilderState._initializeSignal seems to call Computed.value ([1]), which updates the following fields of the Computed instance: _value, _previousValue and _hasPreviousValue.

Here is _SignalBuilderState._initializeSignal :

void _initializeSignal() {
  disposeFn = createEffect((dispose) {
    value = widget.signal(); // [1]
    if (initialized) setState(() {});
    initialized = true;
  });
}