nank1ro / solidart

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

Fix/solid signal computed #55

Closed nank1ro closed 1 year ago

nank1ro commented 1 year ago
codecov[bot] commented 1 year ago

Codecov Report

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

:exclamation: Current head 25a4bbb differs from pull request most recent head c512acd. Consider uploading reports for the commit c512acd to get more accurate results

Impacted file tree graph

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

What is the benefit of createComputed returning Computed instead of ReadSignal?

nank1ro commented 1 year ago

@manuel-plavsic Previously I had done a workaround to allow you to get a Computed with the ReadSignal type but then I found that this workaround did not work in release mode. In Solid to get a provider I do a strict type comparison so ReadSignal<T> != Computed<T>. I am beginning to think, however, that this is not good enough for the SolidProviders. I should be able to get an implementation or extension of a base class

I could implement the comparison with the is keyword but I will introduce some problems:

context.get<ReadSignal<T>>() 

will return the first ReadSignal, Computed, Signal or Resource found in the Solid because each of them extend a ReadSignal. Of course using the identifier this problem doesn't occur. While for SolidProviders this will be better because I could do:

context.get<ClassInterface>(); 

and I can get ClassImplementation.

manuel-plavsic commented 1 year ago

I understand the problematic. This happens because of the provider pattern: type safety is lost automatically. I am in favor of comparing with is together with IDs when needed, but not forcing users to use IDs for every signal. E.g. if context.get<ReadSignal<int>>() is called and there are multiple provided SignalBase instances of the same generic type (e.g. Signal<int> and Computed<int>), then IDs should be used to be sure to get the right one.

I also think that having a separate Computed type is beneficial, so that IDs are needed even less (since one will likely not call context.get<ReadSignal<T>>(); instead, they will call context.get<Signal<T>>() or context.get<Computed<T>>()).

I agree: using is would be good for solid providers; sometimes we want to abstract away some implementation details, and using the interface or the super class might make more sense.