rodydavis / signals.dart

Reactive programming made simple for Dart and Flutter
http://dartsignals.dev
Apache License 2.0
438 stars 50 forks source link

Watch doesn't rebuild under a Streambuilder #192

Closed jgirardet closed 7 months ago

jgirardet commented 7 months ago

Hello, My Widget under Watch doesn't rebuild if I use StreamBuilder before:

      home:
           StreamBuilder(
            stream: FirebaseAuth.instance.authStateChanges(),
            builder: (context, snapshot) {
              if (snapshot.hasData) {
                return const ChatScreen();
              }
              return LoginScreen();
            },
          ),
          // LoginScreen(),
    );

and the widget (some parts are cut for brievety):

import 'package:flutter/material.dart';
import 'package:signals/signals_flutter.dart';

class LoginScreen extends StatelessWidget {
  LoginScreen({super.key});

  final alreadyHasAccount = signal(true);

  late final accountText = computed(() => alreadyHasAccount.value
      ? "No Account ? Signup"
      : "Already account ? signin !");

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Container(
           // cut
                  Watch((context) {
                    return TextButton(
                      onPressed: () {
                        alreadyHasAccount.value = !alreadyHasAccount.value;
                      },
                      child: Text(accountText.value),
                    );
                  }),

If I bypass StreamBuilder, it works.

Thanks for this great package.

nowfalsalahudeen commented 7 months ago

change it to stateful widget and let see, i have faced the similar issue in stateless widget

rodydavis commented 7 months ago

Can you create a complete code example or example unit test? That way I can see if it is a bug.

jgirardet commented 7 months ago

here it is. flutter, signals current version. Version with stream doesn't increment

import 'dart:async';
import 'package:flutter/material.dart';
import 'package:signals/signals_flutter.dart';

void main() {
  runApp(const MyApp());
}

Stream<void> getStream() async* {
  while (true) {
    await Future.delayed(const Duration(seconds: 3));
    yield null;
  }
}

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  // This widget is the root of your application.
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
        title: 'Flutter Demo',
        home: Scaffold(
          body: Column(
            mainAxisAlignment: MainAxisAlignment.spaceEvenly,
            children: [
              Screen("Without Stream"),
              StreamBuilder(
                  stream: getStream(),
                  builder: (_, __) => Screen("With Stream"))
            ],
          ),
        ));
  }
}

class Screen extends StatelessWidget {
  Screen(this.title, {super.key});
  final String title;
  final val = signal(1);

  @override
  Widget build(BuildContext context) {
    return Center(
      child: Column(
        mainAxisAlignment: MainAxisAlignment.center,
        children: [
          Watch(
            (_) => Text(
              val.value.toString(),
            ),
          ),
          ElevatedButton(
            onPressed: () => val.value += 1,
            child: Text(title),
          )
        ],
      ),
    );
  }
}
jinyus commented 7 months ago

Use a stateful widget for Screen A Stateless widget will lose all of its state when rebuilt so the val signal is a new instance everytime it rebuilds.

jgirardet commented 7 months ago

I don't think it's only a matter of rebuild. I modified the previous code with some print debug. The stream ticks every 10 seconds (1 sec was foolish).

With stream case:

I don't see the probleme with stateless widget since it works the first time.

import 'dart:async';
import 'package:flutter/material.dart';
import 'package:signals/signals_flutter.dart';

void main() {
  runApp(const MyApp());
}

Stream<void> getStream() async* {
  while (true) {
    await Future.delayed(const Duration(seconds: 10));
    print("stream ticks");
    yield null;
  }
}

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  // This widget is the root of your application.
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
        title: 'Flutter Demo',
        home: Scaffold(
          body: Column(
            mainAxisAlignment: MainAxisAlignment.spaceEvenly,
            children: [
              Screen("Without Stream"),
              StreamBuilder(
                  stream: getStream(),
                  builder: (_, __) => Screen("With Stream"))
            ],
          ),
        ));
  }
}

class Screen extends StatelessWidget {
  Screen(this.title, {super.key});
  final String title;
  final val = signal(1);

  @override
  Widget build(BuildContext context) {
    print("widget $title rebuild");
    return Center(
      child: Column(
        mainAxisAlignment: MainAxisAlignment.center,
        children: [
          Watch((_) {
            print("text rebuild in $title with value ${val.value}");
            return Text(val.value.toString());
          }),
          ElevatedButton(
            onPressed: () {
              print("pressed in $title new value: ${val.value}");
              val.value += 1;
            },
            child: Text(title),
          )
        ],
      ),
    );
  }
}
jinyus commented 7 months ago

That does look like a bug, but I'd categorize it as undefined behavior. Using signals like that is a giant memory leak so it's hard to debug the root cause.

If you convert Screen to a stateful widget you'll see that it works fine regardless of how often the stream ticks.

jgirardet commented 7 months ago

So maybe the use of statelesswidget should be discouraged in the docs ? I mean at first glance, I thought Watch allowed us to use to use only stateless widget like in getx.

jinyus commented 7 months ago

You can use Stateless widgets, you just can't create signals in them.

This will be possible when/if "useSignal" is added to the package.

jgirardet commented 7 months ago

Ok just a last question If you don't mind (I'll close this issue anyway). In my mind: Streambuilder Build a widget > widget creates signal > Stream changes> StreamBuilder remove the widget > the signal is removed since created in the widget > a new widget is created > a new signal is created. So I don't see where is the memory leak since at the end statefull or statelesswidget are removed with signals created inside them.