Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NotifierProvider override can't be replaced #2458

Closed
passsy opened this issue Apr 12, 2023 · 19 comments
Closed

NotifierProvider override can't be replaced #2458

passsy opened this issue Apr 12, 2023 · 19 comments
Labels
bug Something isn't working needs triage

Comments

@passsy
Copy link
Contributor

passsy commented Apr 12, 2023

I try to exchange an overridden NotifierProvider. But I'm not able to access the new object after updateOverrides.

import 'package:riverpod/riverpod.dart';
import 'package:riverpod/src/internals.dart';
import 'package:test/test.dart';

import '../utils.dart';

void main() {
  test(
    'NotifierProvider override can be updated',
    () async {
      final count = NotifierProvider<Notifier<int>, int>(
        () => throw 'Not provided, requires override',
      );

      final container = createContainer(
        overrides: [
          count.overrideWith(Counter.new),
        ],
      );
      expect(container.read(count), 0);

      container.updateOverrides([count.overrideWith(AlwaysFive.new)]);
      await container.pump();
      expect(
        container.read(count.notifier).runtimeType,
        AlwaysFive, // Fail: Actual: Type:<Counter>
      );
      expect(container.read(count), 5); // Fail: Actual: 0
    },
  );
}

class Counter extends Notifier<int> {
  @override
  int build() {
    return 0;
  }

  void increment() {
    state++;
  }
}

class AlwaysFive extends Notifier<int> {
  @override
  int build() {
    return 5;
  }
}
@passsy passsy added bug Something isn't working needs triage labels Apr 12, 2023
@rrousselGit
Copy link
Owner

That's how overrideWith / overrideWithProvider works.

Once the value is instantiated, replacing the provider is no-op until the provider is fully disposed (autoDispose). Then and only then is the new override considered

A different behavior would be problematic, as overrides are often created inside the build method of widgets.
Meaning the override may be updated even if nothing actually changed.
We wouldn't want this to destroy the stage of providers.

@passsy
Copy link
Contributor Author

passsy commented Apr 12, 2023

Does this mean I can't use riverpod as service locator and exchange dependencies on the fly?

I'm struggling to implement a feature flag, that switches an implementation.

import 'package:flutter/material.dart' hide Listener;
import 'package:flutter_riverpod/flutter_riverpod.dart';
import 'package:flutter_test/flutter_test.dart';

void main() {
  testWidgets('Switch override with feature flag', (tester) async {
    final count = NotifierProvider<Notifier<int>, int>(
      () => throw 'Not provided, requires override',
    );

    final featureFlag = StateProvider<bool>((ref) => false);

    await tester.pumpWidget(
      ProviderScope(
        overrides: [
          count.overrideWith(() {
            // How to switch from one instance to another? ref doesn't exist
            // final bool flag = ref.watch(featureFlag.notifier);
            // if (flag) {
            //   return AlwaysFive();
            // }
            return Counter();
          }),
        ],
        child: MaterialApp(
          home: Consumer(
            builder: (context, ref, child) {
              return Scaffold(
                body: Text(ref.watch(count).toString()),
                floatingActionButton: FloatingActionButton(
                  onPressed: () {
                    ref.read(featureFlag.notifier).state = true;
                  },
                ),
              );
            },
          ),
        ),
      ),
    );

    expect(find.text('0'), findsOneWidget);
    await tester.tap(find.byType(FloatingActionButton));
    await tester.pump();
    expect(find.text('5'), findsOneWidget);
  });
}

class Counter extends Notifier<int> {
  @override
  int build() {
    return 0;
  }

  void increment() {
    state++;
  }
}

class AlwaysFive extends Notifier<int> {
  @override
  int build() {
    return 5;
  }
}

I expect that all providers that depend on my count Provider to rebuild when the instance changes.

@rrousselGit
Copy link
Owner

Do:

class Counter extends Notifier<int> {
  @override
  int build() {
    if (ref.watch(flagProvider)) return 5;
    return 0;
  }

@passsy
Copy link
Contributor Author

passsy commented Apr 12, 2023

What if Counter lives in a different package and has no access to flagProvider?

This is the solution with package:provider I'd like to replicate with riverpod.

Note: The Counter impls have no reference to the feature flag

import 'package:flutter/material.dart' hide Listener;
import 'package:flutter_test/flutter_test.dart';
import 'package:provider/provider.dart';

void main() {
  testWidgets('Switch override with feature flag', (tester) async {
    final featureFlag = ValueNotifier(false);

    await tester.pumpWidget(
      ListenableProvider<ValueNotifier<bool>>.value(
        value: featureFlag,
        builder: (context, child) {
          final counter = context.watch<ValueNotifier<bool>>().value
              ? AlwaysFive()
              : CounterImpl();

          return ChangeNotifierProvider<Counter>.value(
            value: counter,
            builder: (context, _) {
              return Builder(
                builder: (context) {
                  return MaterialApp(
                    home: Scaffold(
                      body: Text(context.watch<Counter>().count.toString()),
                      floatingActionButton: FloatingActionButton(
                        onPressed: () {
                          context.read<ValueNotifier<bool>>().value = true;
                        },
                      ),
                    ),
                  );
                },
              );
            },
          );
        },
      ),
    );

    expect(find.text('0'), findsOneWidget);
    await tester.tap(find.byType(FloatingActionButton));
    await tester.pump();
    expect(find.text('5'), findsOneWidget);
  });
}

abstract class Counter extends ChangeNotifier {
  int get count;

  void increment();
}

class CounterImpl extends ChangeNotifier implements Counter {
  int _count = 0;

  @override
  int get count => _count;

  @override
  void increment() {
    _count++;
    notifyListeners();
  }
}

class AlwaysFive extends ChangeNotifier implements Counter {
  @override
  int get count => 5;

  @override
  void increment() {}
}

@rrousselGit
Copy link
Owner

Your provider sample is flawed. You shouldn't create notifiers inside the build method like this. Any rebuild would destroy the state.

But anyway, you cannot change the Notifier instance live. Not much else to say.

But I don't see why you would have to use such an architecture. What are you trying to do exactly? You've only talked about the solution you're trying to use, not the problem you're trying to solve.

@passsy
Copy link
Contributor Author

passsy commented Apr 12, 2023

Your provider sample is flawed. You shouldn't create notifiers inside the build method like this. Any rebuild would destroy the state.

This is just a sample. The real code saves the instance in a StatefulWidget

The real-world use case: Switch between two Auth implementations: The real one (in a 3rd party package) and a Fake version that tells the app the user is always signed in.

The user presses a Button, I want to change the instance of Auth throughout the app and jump the login screen

@rrousselGit
Copy link
Owner

You haven't described a problem but a solution.
I'm sure you could implement whatever you're trying to implement without switching Notifier implementation.

I'm asking for the actual problem, the business one. Because chances are there's a different path possible.

But anyway, if you absolutely want to use the OOP path, you can use the delegate pattern

@riverpod
class YourNotifier extends _$YourNotifier {
  YourNotifierDelegate delegate;

  @override
  Widgwt build() => ref.watch(delegateProvider).build();

  void increment() => ref.watch(delegateProvider).build();
}

abstract class YourDelegate { /* Not a Notifier */
  int build();
  void increment();
}

@riverpod
YourDelegate delegateProvider(DelegateProviderRef ref) {
  if (ref.watch(flagProvider)) return Always5();
  return DefaultImpl();
}

@passsy
Copy link
Contributor Author

passsy commented Apr 12, 2023

Sure I can create my own delegate. But isn't riverpod like the ultimate delegate? I expected it to offer such a feature. And it does work for normal Providers.

What bugs me is that the pattern is working fine with normal Provider (except that it doesn't notify when the Notifier changes), but not with NotifierProvider. What's the reason that it doesn't have a ref argument?

import 'package:flutter/material.dart' hide Listener;
import 'package:flutter_riverpod/flutter_riverpod.dart';
import 'package:flutter_test/flutter_test.dart';

void main() {
  testWidgets('Switch override with feature flag', (tester) async {
    final featureFlag = StateProvider<bool>((ref) => false);
    // Works but doesn't notify when Counter changes
    final count = Provider<Counter>((ref) {
      final flag = ref.watch(featureFlag);
      if (flag) {
        return AlwaysFive();
      }
      return CounterImpl();
    });

    // Can't subscribe to featureFlag
    final notifierCount = NotifierProvider<Counter, int>(() {
      // ERROR: no ref, 
      final flag = ref.watch(featureFlag);
      if (flag) {
        return AlwaysFive();
      }
      return CounterImpl();
    });

    await tester.pumpWidget(
      ProviderScope(
        child: MaterialApp(
          home: Consumer(
            builder: (context, ref, child) {
              return Scaffold(
                body: Text(ref.watch(count).count.toString()),
                floatingActionButton: FloatingActionButton(
                  onPressed: () {
                    ref.read(featureFlag.notifier).state = true;
                  },
                ),
              );
            },
          ),
        ),
      ),
    );

    expect(find.text('0'), findsOneWidget);
    await tester.tap(find.byType(FloatingActionButton));
    await tester.pump();
    expect(find.text('5'), findsOneWidget);
  });
}

abstract class Counter implements Notifier<int> {
  void increment();
  int get count;
}

class CounterImpl extends Notifier<int> implements Counter {
  @override
  int build() {
    return count;
  }

  @override
  void increment() {
    state++;
    count++;
  }

  @override
  int count = 0;
}

class AlwaysFive extends Notifier<int> implements Counter {
  @override
  int build() {
    return 5;
  }

  @override
  void increment() {}

  @override
  int get count => 5;
}

@passsy
Copy link
Contributor Author

passsy commented Apr 12, 2023

Unfortunately, the delegate pattern also doesn't work because NotifierProviderRef<State> get ref => _element; is never set on the delegate. I can't set it because it is private. build() therefore fails because it use ref in the build method of the delegate.

@riverpod
class YourNotifier extends _$YourNotifier {
  YourNotifierDelegate delegate;

  @override
  Widgwt build() => ref.watch(delegateProvider).build();

  void increment() => ref.watch(delegateProvider).build();
}

abstract class YourDelegate { /* Not a Notifier */
  int build();
  void increment();
}

@riverpod
YourDelegate delegateProvider(DelegateProviderRef ref) {
  if (ref.watch(flagProvider)) return Always5();
  return DefaultImpl();
}
#0      BuildlessNotifier._element (package:riverpod/src/notifier/base.dart)
#1      BuildlessNotifier.ref (package:riverpod/src/notifier/base.dart:17:41)
#2      AmplifyAuth.build (package:smars/features/auth/application/amplify_auth.dart:22:5)
#3      FeatureFlaggedAuth.build (package:smars/features/auth/application/auth.dart:60:22)
#4      NotifierProviderImpl.runNotifierBuild (package:riverpod/src/notifier/base.dart:159:38)
#5      NotifierProviderElement.create (package:riverpod/src/notifier/base.dart:199:23)
#6      ProviderElementBase.buildState (package:riverpod/src/framework/element.dart:425:7)
#7      ProviderElementBase.mount (package:riverpod/src/framework/element.dart:248:5)

@passsy
Copy link
Contributor Author

passsy commented Apr 12, 2023

Got it working!

The trick is to create a ProviderScope that listens to the featureFlag and injects the overrides accordingly into the widget tree. This is where the switch happens. But instead of keeping the ProviderScope and trying to switch the overrides, I also change the ProviderScope.key and force the widget the be recreated.

This example now switches between AlwaysFive and CounterImpl when the FloatingActionButton is clicked.

import 'package:flutter/material.dart' hide Listener;
import 'package:flutter_riverpod/flutter_riverpod.dart';
import 'package:flutter_test/flutter_test.dart';

void main() {
  testWidgets('Switch override with feature flag', (tester) async {
    final featureFlag = StateProvider<bool>((ref) => false);
    final counter = NotifierProvider<Counter, int>(
        () => throw 'Not provided, requires override');

    await tester.pumpWidget(
      ProviderScope(
        child: MaterialApp(
          home: Consumer(
            builder: (context, ref, child) {
              final flag = ref.watch(featureFlag);
              return ProviderScope(
                key: flag ? const Key('fake') : const Key('real'),
                overrides: [
                  if (flag)
                    counter.overrideWith(AlwaysFive.new)
                  else
                    counter.overrideWith(CounterImpl.new)
                ],
                child: Consumer(
                  builder: (context, ref, child) {
                    return Scaffold(
                      body: Text(ref.watch(counter).toString()),
                      floatingActionButton: FloatingActionButton(
                        onPressed: () {
                          ref.read(featureFlag.notifier).state = true;
                        },
                      ),
                    );
                  },
                ),
              );
            },
          ),
        ),
      ),
    );

    expect(find.text('0'), findsOneWidget);
    await tester.tap(find.byType(FloatingActionButton));
    await tester.pump();
    expect(find.text('5'), findsOneWidget);
  });
}

abstract class Counter implements Notifier<int> {
  void increment();
  int get count;
}

class CounterImpl extends Notifier<int> implements Counter {
  @override
  int build() {
    return count;
  }

  @override
  void increment() {
    state++;
    count++;
  }

  @override
  int count = 0;
}

class AlwaysFive extends Notifier<int> implements Counter {
  @override
  int build() {
    return 5;
  }

  @override
  void increment() {}

  @override
  int get count => 5;
}

@rrousselGit
Copy link
Owner

I told you that the delegate should not be a notifier though.

But anyway you didn't answer my question still.
What's the problem you're trying to solve? So far you're only focused on one potential solution to said problem.

The delegate pattern I gave is only a workaround to forcibly what you think you should do.
If you want a proper way to solve your problem, I need to know what it truly is.

You clearly don't want to make a counter with a custom implementation which always returns 5.

@passsy
Copy link
Contributor Author

passsy commented Apr 12, 2023

Not sure what I can add to the real-world problem other than what I stated.
We want to switch between two authentication services on the fly.

We have a user service that manages the whole login flow, provided by a third party. Our app is still in the prototype phase and works without a server. Auth is fully integrated, but not working due to some backend configuration.
We added a "skip login" button on the login screen. When tapped, we use our FakeAuthService that basically tells the app the user is signed in. Users are able to completely use the prototype.

It's important that the real AuthService and the FakeAuthService have the same interface and that the app doesn't know about the actual implementation. We don't want to change anything when we remove the "skip login" button in the future.

It's not that different from the counter. Instead of a counter that returns always 5, I get a FakeAuthService that always tells me isSignedIn = true and accessToken = 'invalid token'.

@rrousselGit
Copy link
Owner

What about mocking the network requests instead? Such as using an interceptor and returning a pre-set response? Then you'd stick to using the official AuthService, but with predefined responses.

@passsy
Copy link
Contributor Author

passsy commented Apr 12, 2023

I really appreciate that you want to find alternative solutions and think outside the box, seriously!

The auth package I hide behind the interface is from a 3rd party. It's very complex and I don't know the internals. Mocking responses might be hard at best or impossible due to signing and encryption of the responses. Either way a nightmare to maintain when it changes and way to much code for what I try to achieve.

Hiding complex code behind an interface, and being able to swap the actual implementation is a common practice. I think we agree on that.

The FakeAuth implementation of that interface is small and trivial and well-encapsulated. The separation of the fake implementation and the actual implementation is a good thing in my opinion.

It all works as I want it to, except for when I use Notifier. Then I have to come up with the workaround above.

When I switch it to ChangeNotifier it works fine, I can build the switch.

I opened a new issue where I show the API differences between ChangeNotifierProvider and NotifierProvider in detail. The issue has shifted a bit and it might be easier for others to follow with a new issue.

Thanks for your time as I was discovering the internals of riverpod!

@rrousselGit
Copy link
Owner

I'm not sure why it would be a nightmare to mock the requests.
If you can't mock it through your HTTP client, you can mock it through a custom Service API which could easily be exposed with a simple Provider (which can use flagProvider).

That's how most people do it. They don't replace the Notifier, they replace a service dependency.

@passsy
Copy link
Contributor Author

passsy commented Apr 13, 2023

Again, the Auth class is 3rd party. It doesn't allow injecting anything, neither a HTTP client or an API. That's fine, because it has a well-defined interface that is easy to be mocked, as we do in our tests all the time. All I was struggling with is swapping the Notifier in the container after it was created.

Also, responses like JWTs can't be mocked easily. They expire and can't be created without private key. The 3rd party Auth class would have to use clock allowing me to go back in time to the point where the token was still valid.

The only mistake that was made is that the Auth class uses Notifier<AuthState> instead of StateNotifier<AuthState> which can be swapped easily.
I checked the code, it doesn't use ref in its build() method. So it could be changed from Notifier to StateNotifier.

@rrousselGit
Copy link
Owner

Do you mean that Auth is 3rd party, but it's also a Riverpod notifier?

Auth being a 3rd party or not doesn't change much. You can make a wrapper around it if needed, to recover the ownership of the class and update whatever you need to.

I checked the code, it doesn't use ref in its build() method. So it could be changed from Notifier to StateNotifier.

StateNotifier isn't supported by the new code-gen syntax though. You're not using it yet, but it'll replace the old syntax once metaprogramming is out.
I'd suggest finding a different solution.

@passsy
Copy link
Contributor Author

passsy commented Apr 13, 2023

Do you mean that Auth is 3rd party, but it's also a Riverpod notifier?

Correct.

I'd suggest finding a different solution.

That's a task for Future Pascal, earliest next year 🚀

@rrousselGit
Copy link
Owner

Honestly, I'd use the delegate variant (but don't make the delegate implement Notifier as I said before)

It's a pretty common pattern. Sure that's more tedious than you'd like, I get that. But it's also a pretty rare use case.

To begin with, it's apparently not something you want to ship in release mode.
If it's just for debugging, you could opt for a simpler solution and either always or never override the provider for the lifetime of the app. I'm skeptical about whether you truly need this "flagProvider"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs triage
Projects
None yet
Development

No branches or pull requests

2 participants