escamoteur / watch_it

MIT License
103 stars 8 forks source link

allReady called multiple times #23

Closed exaby73 closed 5 months ago

exaby73 commented 6 months ago

I'm using allReady in a splash page, to then navigate to the next screen. Using a simple print while I was implementing this, I found that the print was called multiple times. I'm using injectable for convenience. Here is the generated code from injectable:

extension GetItInjectableX on _i1.GetIt {
// initializes the registration of main-scope dependencies inside of GetIt
  _i1.GetIt init({
    String? environment,
    _i2.EnvironmentFilter? environmentFilter,
  }) {
    final gh = _i2.GetItHelper(
      this,
      environment,
      environmentFilter,
    );
    final externalDependencies = _$ExternalDependencies();
    gh.singleton<_i3.AppRouter>(_i3.AppRouter());
    gh.singletonAsync<_i4.SharedPreferences>(
        () => externalDependencies.sharedPreferences);
    gh.singletonAsync<_i5.AuthModel>(() async =>
        _i5.AuthModel(await getAsync<_i4.SharedPreferences>())..init());
    return this;
  }
}

class _$ExternalDependencies extends _i6.ExternalDependencies {}

Splash page:

class SplashPage extends StatelessWidget with WatchItMixin {
  const SplashPage({super.key});

  @override
  Widget build(BuildContext context) {
    allReady(
      onReady: (context) {
        // TODO: Navigate
        print('Ready'); // Being called 3 times
      },
    );

    return const Scaffold(
      body: Center(
        child: Text('Splash'),
      ),
    );
  }
}

Logs:

flutter: Not yet ready objects:
flutter: [Object __environments__filter__ has not completed, Object __environments__ has not completed, Registered object of Type SharedPreferences has not completed, Registered object of Type AuthModel has not completed]
flutter: Ready
flutter: Ready
flutter: Ready

Let me know if there is any other information you need

exaby73 commented 6 months ago

For anyone wanting a workaround, you can try something like this:

import 'package:flutter/material.dart';
import 'package:watch_it/watch_it.dart';

bool _navigating = false;

class SplashPage extends WatchingStatefulWidget {
  const SplashPage({super.key});

  @override
  State<SplashPage> createState() => _SplashPageState();
}

class _SplashPageState extends State<SplashPage> {
  @override
  void initState() {
    super.initState();
    _navigating = false;
  }

  @override
  Widget build(BuildContext context) {
    allReady(
      onReady: (context) {
        if (_navigating) return;
        _navigating = true;
        print('Ready');
      },
    );

    return const Scaffold(
      body: Center(
        child: Text('Splash'),
      ),
    );
  }
}
escamoteur commented 6 months ago

Actually you don't use allReady() the right way It returns a Future and is meant to be used with a FutureBuilder

exaby73 commented 5 months ago

I want to run a side effect like Navigating. Don't think it's ideal to run it in the builder function of FutureBuilder.

The way I did it is documented here

escamoteur commented 5 months ago

Ah, now I understand but in your example you did do the navigation in that handler Am 12. Jan. 2024, 12:08 +0100 schrieb Nabeel Parkar @.***>:

I want to run a side effect like Navigating. Don't think it's ideal to run it in the builder function of FutureBuilder. The way I did it is documented here — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: @.***>

exaby73 commented 5 months ago

Yes, I did. I added a print to easily see the 2 prints in the console

escamoteur commented 5 months ago

ok, what I normally do is call allReady in the initState with a following then where I do the navigation. As the then is called a bit later than the initstate the context will be valid then

exaby73 commented 5 months ago

Please take the following as constructive criticism / suggestions:

I do not think this is good DX. I think the point of most state management solutions is to avoid writing boilerplate-y StatefulWidgets in the first place. I would be okay with watch_it not having life-cycle hooks if it was compatible with flutter_hooks for example but it doesn't seem like the case, even when using the mixins with HookWidget.

While watch_it does technically have most if not all features you need from a state management solution, for larger apps, that boilerplate for StatefulWidgets count a lot. My ideal setup uses flutter_bloc for ease of state management, get_it and injectable for DI, and flutter_hooks for convenience on local widget state and lifecycle hooks. I think watch_it has potential. It already can be a replacement for flutter_bloc, it already uses get_it, and by extension, injectable is compatible out of the box, and with a (possible) future addition of lifecycle hooks, can be a replacement for flutter_hooks. Local widget state can also be an area where watch_it could have something similar to useState, eliminating the need for StatefulWidgets in most situations altogether. If these are addressed, watch_it can become an all-in-one state management solution.

Maybe some of these make more sense to be in separate packages to not bloat watch_it with extra stuff.

Now that said, the original issue still persists where print is being called twice, forcing a workaround as I mentioned in https://github.com/escamoteur/watch_it/issues/23#issuecomment-1877696783. What we are currently discussing doesn't really relate to the original issue. Maybe a new discussion can be spawned from this in a separate issue.

escamoteur commented 5 months ago

Ah, I finally think I understand your problem. it's not that allReady is called twice but that the handler function that you use is called on every build. Indeed this should be changed. It shouldn't be called again unless allReady was false (after a new registerAsync) again.

escamoteur commented 5 months ago

allReady now has an optional new parameter callHandlerOnlyOnce. also the latest version as a callOnce and ondispose function