nank1ro / solidart

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

[feat] support refreshing in error state #34

Closed manuel-plavsic closed 1 year ago

manuel-plavsic commented 1 year ago

This PR adds support for refreshing while in error state. Docs and tests have been updated to make use of this functionality.

If one does not want to keep displaying the error while refreshing, I guess they can create a custom function, e.g. displayLoading, and call it both in the ready and loading callbacks, e.g.:

ResourceBuilder(
  resource: user,
  builder: (_, userValue) {
    Widget displayLoading() {
      // ...
    }

    return userValue.on(
      ready: (data, refreshing) {
        if (refreshing) {
          // ... display data with CircularProgressIndicator
        } else {
          // ...
        }
      },
      error: (error, _, refreshing) {
        if (refreshing) {
          return displayLoading();
        } else {
          // ...
        }
      },
      loading: displayLoading;
      },
    );
  }
);

I made some small changes:

What do you think of this functionality? Also, do you know why a test I marked with // TODO: should be errorFinder(refreshing: true) fails with refreshing:true (maybe it is correct so, I am not fully sure)?

nank1ro commented 1 year ago

I am beginning to think about a slight change that will improve utilization, however. Let's remove the refreshing value from the callback and put it simply accessible from the resource state.

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

In this way, the isRefreshing value can be present in both the ready and error states but easily used only when needed

So your example would become:

ResourceBuilder(
  resource: user,
  builder: (_, userState) {
    if (userState.isRefreshing) return LoadingIndicator();

    return userState.on(
      ready: (data) {
        return DataView(data);
      },
      error: (error, stackTrace) {
       return ErrorView(error);
      },
      loading: () => LoadingIndicator();
      },
    );
  }
);

This will still allow to view data and refreshing together:

ResourceBuilder(
  resource: user,
  builder: (_, userState) {
    return userState.on(
      ready: (data) {
        return Stack(
           children: [
             DataView(data),
             if (userState.isRefreshing) LoadingIndicator().
           ],
         ),
      },
      error: (error, stackTrace) {
       return ErrorView(error);
      },
      loading: () => LoadingIndicator();
      },
    );
  }
);
codecov[bot] commented 1 year ago

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff           @@
##             dev      #34   +/-   ##
======================================
  Coverage       ?   97.59%           
======================================
  Files          ?       17           
  Lines          ?      707           
  Branches       ?        0           
======================================
  Hits           ?      690           
  Misses         ?       17           
  Partials       ?        0           
manuel-plavsic commented 1 year ago

I agree with you that using resource.state.isRefreshing feels like the better approach.

I implemented the changes! 🎉

Note: I had to replace Future.microtask with Future.delayed(Duration(...)) and add a duration to tester.pumpAndSettle to make one test pass correctly. The description of the test is "ResourceBuilder widget works properly in ResourceError state"

manuel-plavsic commented 1 year ago

Done :)