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

feat/v2.0.0 #85

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

feat/v2.0.0 #85

wants to merge 5 commits into from

Conversation

nank1ro
Copy link
Owner

@nank1ro nank1ro commented Jan 2, 2024

  • CHORE: Improved Solid widget performance by more than 3000% in finding ancestor providers.
    During my performance tests, before I was able to observe up to 71 signals (provided through Solid) per second, now this numbers increased to 2159 signals per second.

  • FEAT: The SignalBuilder widget now automatically tracks the Signals used in the builder function allowing you to react to any number of signals at the same time.
    Before:

    SignalBuilder(
      signal: counter,
      builder: (context, value, child) {
        return Text('$value');
      },
    ),

    Now:

    SignalBuilder(
      builder: (context, child) {
        return Text('${counter.value}');
      },
    ),
  • BREAKING CHANGE: Removed DualSignalBuilder and TripleSignalBuilder in favor of SignalBuilder.

  • BREAKING CHANGE the context.observe methods (due to the performance improvements of the Solid widget) now needs the type of signal
    Before:

    final counter = context.observe<int>();

    Now:

    final counter = context.observe<int, Signal<int>>();
    // or
    final counter = context.observeSignal<int>();

    So this change includes the new observeSignal, observeReadSignal, observeComputed, observeResource, observeListSignal, observeSetSignal and observeMapSignal

    The PR has still some problems:

    1. The auto dispose of signals happens when using SignalBuilder because the effect tracks the dependencies in the build method. This is necessary to make it performant but, for a little moment, the signals will not be watched by the Effect. I still need to find a way to fix this.
    2. I don't know if context.observe<T, S> still be exposed, due to the new specific observe methods.
    3. Still waiting for the v2 of SolidJS that may happen at the end of January 2024, this could improve the performance of the automatic tracking system significantly (https://github.com/bubblegroup/bubble-reactivity)

    TODOs

    • fix unit tests
    • update solidart_lint
    • update docs

@nank1ro nank1ro added the feature New feature or request label Jan 2, 2024
@nank1ro nank1ro self-assigned this Jan 2, 2024
@jinyus
Copy link

jinyus commented Jan 2, 2024

Note that SignalBuilder has a hidden footgun that will catch some users. Any signals accessed in its entire subtree will trigger a rebuild in the parent.

SignalBuilder
  |_ Container
      |_ Column
            |_ SizedBox
                 |_ SignalBuilder
                       |_ Text  <- signal watched here will rebuild both SignalBuilders

@nank1ro
Copy link
Owner Author

nank1ro commented Jan 2, 2024

Note that SignalBuilder has a hidden footgun that will catch some users. Any signals accessed in its entire subtree will trigger a rebuild in the parent.

SignalBuilder

  |_ Container

      |_ Column

            |_ SizedBox

                 |_ SignalBuilder

                       |_ Text  <- signal watched here will rebuild both SignalBuilders

Thanks for reporting 🙏, need to investigate further to fix this

@nank1ro
Copy link
Owner Author

nank1ro commented Jan 2, 2024

@jinyus can't reproduce the issue, this is the code I used to test

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

final a = Signal(0);
final b = Signal(100);

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

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: SignalBuilder(
        builder: (_, __) {
          print('build a');
          return Column(
            children: [
              Text(a().toString()),
              const Second(),
            ],
          );
        },
      ),
      floatingActionButton: Column(
        mainAxisSize: MainAxisSize.min,
        children: [
          TextButton(
            onPressed: () => a.value++,
            child: const Text('add to A'),
          ),
          TextButton(
            onPressed: () => b.value++,
            child: const Text('add to B'),
          ),
        ],
      ),
    );
  }
}

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

  @override
  Widget build(BuildContext context) {
    return SignalBuilder(
      builder: (_, __) {
        print('build b');
        return Text(b().toString());
      },
    );
  }
}

Can you share your code please?

@jinyus
Copy link

jinyus commented Jan 2, 2024

I'm no longer seeing this bug. I will have to investigate further. Consider this a non-issue for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants