nank1ro / solidart

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

[feat] add Wrapped class #27

Closed manuel-plavsic closed 1 year ago

manuel-plavsic commented 1 year ago

The PR adds a Wrapped<T> class. This class is used:

To unwrap a wrapped value, use the getter: wrappedValue.unwrap.

The proposed approach should be preferred over the current T? approach, as T? is not as type-safe (if T is nullable):

I also managed to simplify the comparator type to ValueComparator<T>? (thanks to an added getter, async state transitions always result in updates, e.g. loading -> ready). This was done because of the introduction of Wrapped.

What are your thoughts on this feature? Can you think of a better name than Wrapped? Should i change the class member names ending in value to wrappedValue or similar, in case they are Wrapped instances?

nank1ro commented 1 year ago

I don't like the idea of "wrapping" the value in another class. It's already a bit verbose to write this:

final resource = createResource(..);
final value = resource.state.value;

but changing it to

final value = resource.state.wrappedValue.unwrap;

It's even more verbose.

Can you provide me a simple example of some code that gives you issues? I don't understand this problem in particular: "to distinguish between loading and ready states". The resource has a ResourceState value that already handles the [loading, error, data] states.

manuel-plavsic commented 1 year ago

I see your point as well.

If you look at this method (belonging to ResourceExtensions<T>:

  /// Attempts to synchronously get the value of [ResourceReady].
  ///
  /// On error, this will rethrow the error.
  /// If loading, will return `null`.
  T? get value {
    return map(
      ready: (r) => r.value,
      // ignore: only_throw_errors
      error: (r) => throw r.error,
      loading: (_) => null,
    );
  }

This should be independent of loading, ready and error states. If T is nullable and this method returns null it will be unclear if the async state is data or loading.

Regarding the previous value point, here is a simple example:

class CounterPage extends StatefulWidget {
  const CounterPage({super.key, required this.counter});

  final Signal<int?> counter;

  @override
  State<CounterPage> createState() => _CounterPageState();
}

class _CounterPageState extends State<CounterPage> {
  @override
  void initState() {
    if (widget.counter.previousValue == null) {
      // should do some stuff based on previous value...

      // is its previous valid value null or has counter's value not "
      // "changed yet? a bit unclear...");
    }
    super.initState();
  }

  // ...
}
nank1ro commented 1 year ago

Regarding your first point. That getter is just an extension to get the value synchronously. That doesn't mean you lose the ability to know what state the resource is in. Just use resource.isReady to make sure it is not in loading or error state.

Regarding the second point, we could add a getter of the type hasPreviousValue that is set to true as soon as the value changes the first time. Without complicating it by adding new classes.

manuel-plavsic commented 1 year ago

resource.isReady makes sense.

I like your hasPreviousValue suggestion! It is much better than this wrapper class. I will close this PR and I'll make another one adding this getter.