rrousselGit / state_notifier

ValueNotifier, but outside Flutter and with some extra perks
MIT License
311 stars 28 forks source link

A few state notifiers stopped working, is it due to updateShouldNotify? #58

Closed mark8044 closed 2 years ago

mark8044 commented 2 years ago

I just did a flutter pub upgrade a few hours ago and the only thing I noticed changed was this package.

At the same time a few (not all) of my state notifiers stopped working as expected and reading through I just noticed this was changed https://github.com/rrousselGit/state_notifier/pull/57

Wondering if this is related or if there is any quick statement on how this updateShouldNotify works?

My code that stopped working is:

final paginationProvider = StateProvider.family((ref, id) => true);
  return Container(
          child: Consumer(
          builder: (context, watch, _)
                {

                  _showPaginationBar = ref.watch(paginationProvider(widget.randomHash).state).state;

This provider is updated in another class, when the screen is scrolled as follows:

    if ((_thisScrollDirection == 'down') && (metrics.pixels > _lastTappedScrollPosition+100.0))
    {
      ref.read(paginationProvider(_randomHash).state).state = false;
    }

It doesn't seem to update anymore, maybe I don't need Consumer anymore since it is inside a ConsumerStatefulWidget ? This is some of my most early/old code in my app, but I am unsure if this is the reason?

It all seemed to be working smoothly just before this update

Thanks!

rrousselGit commented 2 years ago

Could you try adding to your pubspec.yaml:

dependencies:
  state_notifier: 0.7.1

to see if the issue disappears?

mark8044 commented 2 years ago

Could you try adding to your pubspec.yaml:

dependencies:
  state_notifier: 0.7.1

to see if the issue disappears?

Yup, all fixed now, problem is gone

mark8044 commented 2 years ago

Id rather stay on the latest versions, do you think this an issue of my poor coding that I could try to fix or a bug and I should wait for an update?

Thanks much

klondikedragon commented 2 years ago

I'm a huge fan of this package and riverpod. I'm having the exact same issue. From what I can tell it's a regression in 0.7.2: It's setting _state to the new value before calling updateShouldNotify:

@protected
  set state(T value) {
    assert(_debugIsMounted(), '');
    if (identical(_state, value)) {
    final previousState = _state;
    _state = value;

    /// only notify listeners when should
    if (!updateShouldNotify(previousState, value)) {
      return;
    }

But then the base implementation of updateShouldNotify compares _state vs current, which will always be true! (_state was already set to the new value before updateShouldNotify is called).

  /// Whether to notify listeners or not when [state] changes
  @protected
  bool updateShouldNotify(
    T old,
    T current,
  ) =>
      !identical(_state, current);

Instead, it needs to compare old vs current. Ideally a test case could be added to ensure that a change notification is triggered when state value changes.

rrousselGit commented 2 years ago

Ah, good catch. Thanks for this explanation

mark8044 commented 2 years ago

This is moving fast--- gotta say I love this community