nank1ro / solidart

Signals in Dart and Flutter, inspired by SolidJS
https://docs.page/nank1ro/solidart
MIT License
155 stars 5 forks source link

Feat/Allow multiple providers of the same Type and allow getting signals by Type #53

Closed nank1ro closed 1 year ago

nank1ro commented 1 year ago

As discussed here this PR enables the ability to have multiple providers of the same type using identifiers.

codecov[bot] commented 1 year ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (dev@6c48502). Click here to learn what that means. The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##             dev      #53   +/-   ##
======================================
  Coverage       ?   96.26%           
======================================
  Files          ?       17           
  Lines          ?      723           
  Branches       ?        0           
======================================
  Hits           ?      696           
  Misses         ?       27           
  Partials       ?        0           
manuel-plavsic commented 1 year ago

It looks very good to me! So far I have a few questions.

nank1ro commented 1 year ago

It looks very good to me! So far I have a few questions.

  • What does SolidSignal.autodispose do exactly? Does it auto dispose in case a signal is no longer being listened to?
  • Also, what does Solid._autoDispose do? Can you also explain to me why Solid.value sets _autoDispose to false.
  1. When the Solid widgets disposes, it disposes all the signals, unless the autodispose value is set to false. This was already present before in SolidSignalOptions and has been moved here to be more similar to SolidProvider.
  2. The naming is very bad here. I'm going to change it in something more appropriate. What it does is not disposing providers and signals when using Solid.value. Otherwise when the Solid widget spawned by Solid.value disposes, we may dispose all the providers and signals being passed to Solid.value, but they are still alive in some other Solid widgets.
manuel-plavsic commented 1 year ago

It looks very good to me! So far I have a few questions.

  • What does SolidSignal.autodispose do exactly? Does it auto dispose in case a signal is no longer being listened to?
  • Also, what does Solid._autoDispose do? Can you also explain to me why Solid.value sets _autoDispose to false.
1. When the `Solid` widgets disposes, it disposes all the signals, unless the `autodispose` value is set to false. This was already present before in `SolidSignalOptions` and has been moved here to be more similar to `SolidProvider`.

2. The naming is very bad here. I'm going to change it in something more appropriate. What it does is not disposing providers and signals when using `Solid.value`. Otherwise when the `Solid` widget spawned by `Solid.value` disposes, we may dispose all the providers and signals being passed to `Solid.value`, but they are still alive in some other `Solid` widgets.
  1. So if it is set to false, if i dispose the Solid widget and I create it again (by rerendering the same page/widget), it will use the signal that was created before. Or will it create a new one, ignoring the previously created one (which still exists, even though it won't be used anymore).

  2. I think I understand this point.

I am a bit unsure about this autoDispose functionality altogether. I think the best approach would be to not include it at all. I think it makes sense to integrate this functionality with global observables (e.g. Riverpod providers), since the state there is not locally scoped and one needs to have a way to auto-dispose global providers at some point in the widget tree to achieve local-state-like semantics.

In our case however, wouldn't it make more sense to lift the SolidSignal up or down in the widget tree? That way one can find the perfect position so that autodispose is no longer needed.

Let me know if what I am saying doesn't make sense. Best would be with an example.

nank1ro commented 1 year ago

@manuel-plavsic If you set autodispose to false, you want to manage the lifecycle of the signal yourself. This field is designed for complex use-cases. You may have the signal in a provider, or you've created it in the initState of the StatefulWidget and you already dispose it yourself in the dispose of the StatefulWidget.

manuel-plavsic commented 1 year ago

In that case, this autodispose actually does exactly what its name implies.

I have just remembered that when I started using Riverpod, I thought that omitting the autoDispose static method on Riverpod providers implied having to dispose the providers yourself. One day I tried hard to dispose Riverpod providers manually, without success however. I think Riverpod providers are just not supposed to be disposed manually, but only through autoDispose. That is why I associated autoDispose keyword in Riverpod to local-state-like functionality .

Hence, I believe that the autodispose parameter is highly beneficial and feels "natural", prompting me to retract my previous statement.

My only suggestion is to rename SolidSignal.autodispose to SolidSignal.autoDispose.