Navigation Menu

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

Add Refreshing constructor to RemoteData ? #9

Open
cbenz opened this issue Jan 5, 2017 · 28 comments
Open

Add Refreshing constructor to RemoteData ? #9

cbenz opened this issue Jan 5, 2017 · 28 comments

Comments

@cbenz
Copy link

cbenz commented Jan 5, 2017

After @krisajenkins article How Elm Slays a UI Antipattern I read a discussion talking about adding a Refreshing a constructor to the RemoteData type.

I need this too! I'd like to display a "spinner" load indicator while data is refreshing (data was already loaded at least once) but continue to display the view.

I have the motivation to implement it, but @krisajenkins I'd just like to know it it's suited.

Thanks for this good library ^^

@krisajenkins
Copy link
Owner

krisajenkins commented Jan 19, 2017

So...just thinking aloud about this. I could add a Refreshing a state. The upside is obvious. The downside is that, for many use-cases, it would force you to handle a case that wasn't relevant. So I got to thinking, what if I represented "refreshing" as a pair of RemoteData types? What would that mean? What possible states could each part be in, and what would each combination signify? For example, if your previous state was Failure, and your current state was Loading, that must mean that something went wrong, but you're in the process of retrying.

No conclusions yet, but here's the table of what that would look like. It seems to me that this reveals useful information... 🤔

|----------------+---------------+--------------------|
| Previous State | Current State | Meaning            |
|----------------+---------------+--------------------|
| Loading        | NotAsked      | Timeout            |
| Loading        | Loading       | Timeout            |
| Loading        | Success a     | Timeout            |
| Loading        | Failure e     | Timeout            |
|----------------+---------------+--------------------|
| Failure e      | NotAsked      | Failure e          |
| Failure e      | Loading       | Retrying           |
| Failure e      | Failure e     | Persistent Failure |
| Failure e      | Success a     | Working again      |
|----------------+---------------+--------------------|
| NotAsked       | NotAsked      | Never asked.       |
| NotAsked       | Loading       | 1st Loading        |
| NotAsked       | Failure e     | 1st Failure        |
| NotAsked       | Success a     | 1st success        |
|----------------+---------------+--------------------|
| Success a      | NotAsked      | Success a          |
| Success a      | Loading       | Refreshing         |
| Success a      | Failure e     | Stale              |
| Success a      | Success a     | Refreshed          |
|----------------+---------------+--------------------|

@cbenz
Copy link
Author

cbenz commented Jan 20, 2017

Thanks for taking the time to reply :-)

I like the idea to represent the data reload as a transition between two state, and not a state.

The downside is that, for many use-cases, it would force you to handle a case that wasn't relevant.

I agree. But one could argue that this could be solved by a map function.

I'm gonna try implementing this idea in my application then we'll see if some patterns emerge.

@robertdp
Copy link

robertdp commented Jan 22, 2017

I had considered using a pair of RemoteData types to accomplish this, and it brought up other questions for me (like if a new Refreshing b value is needed to hold the previous success value and it fails, then should Failure a also be changed to Failure a (Maybe b)?)

One way to solve them seems to be by writing a selection of helper functions, such as:

toError : (RemoteData a b, RemoteData a b) -> Maybe a
toError (current, previous) =
    case current of
        Failure err ->
            Just err
        
        _ ->
            Nothing


isLoading : (RemoteData a b, RemoteData a b) -> Bool
isLoading (current, previous) =
    case current of
        Loading ->
            True
        
        _ ->
            False


toData : (RemoteData a b, RemoteData a b) -> Maybe b
toData (current, previous) =
    case (current, previous) of
        (Success data, _) ->
            Just data
        
        (_, Success data) ->
            Just data
        
        _ ->
            Nothing

Basically shifting the approach from "What is the current value for the remote data?" to "Do I have any data to work with and/or do I have any error to display?"

Any thoughts on this approach?

Some of the considerations are:

  • avoiding creating duplicate requests for data if the current value is Loading
  • no longer needing the previous value (regardless of what it is) as soon as the current value is a Success
  • if the current value is Loading but the previous one is a Failure should you show the error message and the spinner at the same time?

Edit: And the missing piece is some kind of "merging" function. Something like:

merge : RemoteData a b -> (RemoteData a b, RemoteData a b) -> (RemoteData a b, RemoteData a b)
merge new (current, previous) =
    case (new, current) of
        (NotAsked, _) ->
            (current, previous) -- the current value should only ever be NotAsked at initialisation, right?

        (Loading, NotAsked) ->
            (new, previous)
        
        (Success _, Loading) ->
            (new, previous) -- is the previous value worth keeping here?
        
        (Failure _, Loading) ->
            (new, previous)

        _ ->
            (new, current)

@wintvelt
Copy link

wintvelt commented Jan 22, 2017

I am afraid that the transition from one RemoteData state to the next would not solve my issue. I directly save the RemoteData type in my model.
And use it to show a spinner if needed, or show the data in my view.

What is missing for my use case is a discrete state (no transition), in which the data is there, but the app is loading (so I can also show my spinner).

Could an alternative solution be to change the signature of to this?

type RemoteData e a =
    NotAsked
    | Loading (Maybe a)
    | Failure e
    | Success a

That way, one only needs to add the underscore to the loading state, if one does not care whether the data is there:

case remoteData of
    Loading _ ->

And the helper to get the data would be relatively simple:

getData : RemoteData e a -> Maybe a
getData remoteData =
    case remoteData of
        Success data ->
            Just data

        Loading (Just data) ->
            Just data

        _ ->
            Nothing

@robertdp
Copy link

robertdp commented Jan 22, 2017

@wintvelt If you use a pair of (RemoteData, RemoteData) then what you're asking for is achieved with a value of (Loading, Success ...) and the above helpers isLoading and toData.

remoteData = (Loading, Success "whatever the data is")


if isLoading remoteData then
    ...  -- is true in this situation
else
    ...


case toData remoteData of
    Just data ->
        -- and has data in this situation
    
    Nothing ->
        -- handle lack of data

The main issue with RemoteData seems to be that it's not stateful -- it doesn't track any previous values -- but this is exactly how it should be. Tracking state and transitions is important, but that should be a wrapper type for RemoteData (like a pair), not in RemoteData itself.

If you want, why not try a custom type like:

type MyWebData a = MyWebData (RemoteData Http.Error a) (Maybe a)

@wintvelt
Copy link

@robertdp I get that, and I appreciate the upside of non-breaking changes by adding extra helpers.

But I still think that saving previous + new state in the model is an overly complicated solution.
At least for my specific use case, or anyone else who saves RemoteData in their Model.

Changing a from a static state model to a transition state model brings all kinds of nastiness with it: e.g. if a user clicks Load twice, they will still lose their data, unless of course we catch that in our code, and do not update the previous Success a state.
One would need to catch all other possible transition cases from A to B as well.

The current RemoteData structure suggests it represents a static state (NotAsked as example). That is what makes it so simple and attractive.

@robertdp
Copy link

robertdp commented Jan 22, 2017

@wintvelt Do you have an example of a situation that is not handled by the helpers above?

Changing a from a static state model to a transition state model brings all kinds of nastiness with it: e.g. if a user clicks Load twice, they will still lose their data, unless of course we catch that in our code, and do not update the previous Success a state.

I can see that a (Loading, Loading) case in the merge helper might be a good idea.

@robertdp
Copy link

robertdp commented Jan 22, 2017

How about something like:

type StatefulRemoteData e a = StatefulRemoteData (Maybe a) (RemoteData e a)


update : RemoteData e a -> StatefulRemoteData e a -> StatefulRemoteData e a
update remoteData (StatefulRemoteData oldData _) =
    case remoteData of
        Success newData ->
            StatefulRemoteData (Just newData) remoteData
        
        _ ->
            StatefulRemoteData oldData remoteData


empty : StatefulRemoteData e a
empty =
    StatefulRemoteData Nothing NotAsked

which would get used like

empty
    |> update Loading  --> StatefulRemoteData Nothing Loading
    |> update (Success "test")  --> StatefulRemoteData (Just "test") (Success "test")
    |> update Loading  --> StatefulRemoteData (Just "test") Loading
    |> update (Failure "some error")  --> StatefulRemoteData (Just "test") (Failure "some error")

This allows easy pattern matching on the Maybe data with the ability to inspect the current value of the RemoteData as well...

@wintvelt
Copy link

Yeah, that might work.

Perhaps a separation is needed between the request status, i.e. the current RemoteData type, which then arguably would not need the actual data,

And the state of the data, which could simply be there, or not. Regardless of the current state of the Request perhaps.

Under the hood, this could be solved with an (opaque) type of any kind, possibly with a tuple of 2 RemoteData.
But the API is important here. It might require much boilerplate on the importing side, to first translate a Http request and response to a RemoteData type, and then call a helper and store the new type in the model..

@ericgj
Copy link

ericgj commented Jan 22, 2017

To combine the proposals, what about:

type RemoteData e a =
    NotAsked
    | Loading (RemoteData e a)    -- previous state
    | Failure e
    | Success a

@robertdp
Copy link

robertdp commented Jan 23, 2017

@ericgj That would make the following a valid value: Loading (Loading (Loading NotAsked)).

The main point is that RemoteData really represents a single request and its outcome, so going from a Success or Failure value back to NotAsked or Loading implies that it's a new request. This makes any previous data or error irrelevant, because it belongs to the previous request.

I suppose RemoteData (WebData in particular) can be thought of as a HTTP GET request -- the request itself is not aware of previous requests or additional logic, so to handle this there is a higher-level construct that is aware.

Let me just make it clear that I also want the behaviour of Refreshing a, but after some consideration I realised that RemoteData is elegant as it is, and there are other ways to solve this problem.

If you want to track the last Success value, the previous RemoteData value, or even some other functionality like "retry n-times", this can be accomplished by wrapping it in a custom type. If the custom type solves a common problem, then it can also be adopted as a pattern.

type RefreshableWebData a = RefreshableWebData (Maybe a) (WebData a)

type Retry
    = Retry Int
    | Done

type RetryWebData a = RetryWebData (WebData a) Retry

type WebDataHistory a
    = WebDataHistory (WebData a) (WebDataHistory a)
    | EndOfHistory

@krisajenkins I'd be interested to hear your thoughts at this point.

@ericgj
Copy link

ericgj commented Jan 23, 2017

Good points @robertdp, I retract my suggestion. My first thought was just what you said -- wrap it instead of changing RemoteData itself.

@robertdp
Copy link

@ericgj That makes you smarter than me. My first thought was the same as @krisajenkins -- use a pair of RemoteData -- which is a messy and confusing way to do it when we have ADTs at our disposal.

@wintvelt
Copy link

wintvelt commented Jan 23, 2017

After re-reading the thread I agree that RemoteData is (and should remain) about the status of a request. If it would be changed to hold more data, that would destroy much of the simplicity and attractiveness.

What would solve it is maybe not some tuple of new + current request, but simply a way for data to persist, like:

type alias SavedRemoteData e a =
    { latestRequest : RemoteData e a
    , latestData : Maybe a
    }

Either in the library with helper getters and setters, or home-made.

One drawback I see in this approach, is that the actual data is duplicated if the latest request was successful.

There are actually 2 distinct requirements in the thread and the desire for reloading.

  1. Distinguish Loading from Refreshing state (e.g. to make spinner or behavior for reload different from initial load)
  2. Keep data around when a new request is made (possibly also when request fails)

@ericgj
Copy link

ericgj commented Jan 23, 2017

Or just manage it separately in your model.

type alias Model = 
    { pageData : WebData PageData
    , isRefreshing : Bool
    }

view : Model -> Html Msg
view model =
    model.pageData
        |> RemoteData.map (viewPage model.isRefreshing)
        |> withNoSuccess  viewPageNotAsked viewPageLoading viewPageError

viewPage : Bool -> PageData -> Html Msg
viewPage isRefreshing data =
  -- render spinner if isRefreshing, then the rest of the view

-- helper for easier pipelining
withNoSuccess : a -> a -> (x -> a) -> RemoteData x a -> a
withNoSuccess notasked loading failure data =
    case data of
        NotAsked ->
            notasked
        Loading ->
            loading
        Failure x ->
            failure x
        Success a->
            a

Not ideal since it does allow some meaningless states, but there's probably just 3 places in the code where you'd need to touch isRefreshing. You'd have to remember to update isRefreshing at the same time as the RemoteData, but usually you'd just have one 'refresh' action for any given page so not a big deal.

@robertdp
Copy link

robertdp commented Jan 23, 2017

Meaningless states should not be allowed. Especially not after Richard Feldman's talk (I heard that he once clotheslined a nun for allowing invalid states -- true story).

And wrapping it up in a new type lets us use encapsulation to hid all the gritty details away in a module somewhere, making certain that inquisitive developers can't take it to bits and put it back together the wrong way.

@wintvelt
Copy link

wintvelt commented Jan 23, 2017

Agreed that a type is needed which meets Richard Feldman's "making impossible states impossible" test. But it should at the same time also fulfil requirements.

I am not so sure hiding the implementation behind an opaque type is ideal. There is merit in allowing others to do case statements on branches, if they are designed well.

How about:

type SavedRemoteData e a = 
    NeverAsked
    | FirstLoading
    | FirstFailed e
    | HasData a ReloadState

type ReloadState =
    NoReload
    | Reloading
    | Failed e
    | ReloadSuccess

I think this makes impossible states impossible..
And it does not have redundant data.

In this setup, RemoteData e a would be for the request only, not for saving the result.
If you want to save, pass the RemoteData to a helper for SavedRemoteData.
Which would update the SavedRemoteData while preserving any previously saved data..

In view functions, you could ignore the ReloadState if you don't care, or read it to show some spinner.
And perhaps expose a helper function that outputs True if the state is either FirstLoading or HasData _ Reloading.

@robertdp
Copy link

robertdp commented Jan 23, 2017

@wintvelt Both of those types are essentially the same, with the exception of the HasData constructor. Even the semantic difference (first load versus subsequent loads) is fairly arbitrary. And now users have to match against two levels of nested constructors.

Does this accomplish something practical that the RefreshableWebData from earlier doesn't? Surely a single-constructor type is preferred in this case.

@ericgj
Copy link

ericgj commented Jan 23, 2017

Meaningless states should not be allowed.

Sure, but as I'm sure you know, there's no absolutes in programming, just deadlines ;) It's always a tradeoff. Not to say we shouldn't keep looking, as library developers. But as an application developer it's a question of what are we actually talking about. Spinner state I can't really justify hours of design discussion on, as interesting as it is. I'd much rather "control the valid states in the app until something better comes along" than wait for the perfect abstraction, knowing full well it may mean quite a bit of changes later. Or push back on the feature: use Loading and clear the screen when reloading for now, or don't use Loading and drop the spinner. It's rarely an issue stakeholders care deeply about in my experience.

(Not to quell this very fruitful discussion, but I thought it needs to be said from a practical point of view, the quest to make impossible states impossible does not have to block).

@wintvelt
Copy link

Both of those types are essentially the same, with the exception of the HasData constructor.
Does this accomplish something practical that the RefreshableWebData from earlier doesn't? Surely a single-constructor type is preferred in this case.

The trade-off between the different models is: either have more than 1 constructor for loading and failure states, or allow the type to hold the same data twice.

Spinner state I can't really justify hours of design discussion on, as interesting as it is. I'd much rather "control the valid states in the app until something better comes along" than wait for the perfect abstraction. Or push back on the feature: use Loading and clear the screen when reloading for now, or don't use Loading and drop the spinner. It's rarely an issue stakeholders care deeply about in my experience.

Agreed.

@cbenz
Copy link
Author

cbenz commented Jan 24, 2017

I really like the solution with the getData function.

But isn't getData the map function?

What about adding a section in the documentation explaining both approaches: using isRefreshing boolean or a new type, and their pros/cons. That would be a good starting point for a newcomer.

Just for the record in an app I work on I re-created WebData.elm with:

type LoadingStatus a
    = Loading (Maybe a)
    | Loaded a


type WebData a
    = NotAsked
    | Failure Http.Error
    | Data (LoadingStatus a)


getData : WebData a -> Maybe a
getData webData =
    case webData of
        NotAsked ->
            Nothing

        Failure _ ->
            Nothing

        Data loadingStatus ->
            getLoadingStatusData loadingStatus


getLoadingStatusData : LoadingStatus a -> Maybe a
getLoadingStatusData loadingStatus =
    case loadingStatus of
        Loading data ->
            data

        Loaded data ->
            Just data

Now I think that both types WebData and LoadingStatus are heavier than one RemoteData.

@OliverJAsh
Copy link

I'd love to hear how people are getting on with the various solutions in this thread. What works and what doesn't work? In the interim, I'm adding a Refreshing constructor to my remote data type…

@joshhornby
Copy link

I'd be interested to hear the thoughts of @krisajenkins on this. Is it likely that 'refreshing' will make it into this package?

@SidneyNemzer
Copy link

SidneyNemzer commented Mar 8, 2018

For what it's worth, I created my own package for a similar purpose to this: elm-remote-data

It has three "types" of remote data:

  • the basic RemoteData, but without the NotAsked state because I never use that state
  • EditableRemoteData which must be first loaded from the server, but then can be changed locally while tracking what value the server has. It also has a "saving" state saved to the server. Any changes made while saving can still be tracked.
  • CreateableRemoteData may not exist on the server. It can be created locally, then saved to the server. It retains many of the attributes of EditableRemoteData

I successfully used this package in one fairly large project, but I'm still not quite happy with it. At this point, it seems like every project needs its own slightly different RemoteData. For example, maybe some of your data needs a NotAsked state. All of my remote data's share the fact that data is not available before the initial load from the server, but this might not be the case for some projects.

All the variations share similar properties but if they're all different union types, they all need their own set of practically identical utility functions. Also, I'm not very happy with the names I came up with, they're pretty long.

I might look into doing this with extensible records...

@domenkozar
Copy link

domenkozar commented Aug 13, 2019

I wrote a little module that wraps RemoteData to fit the following requirements:

  • only show "is loading" state on the very first request (RemoteData.isLoading rrd.current)
  • don't refresh if previous request is still running, to prevent overloading the db
  • update data only if refreshed request succeeded (avoids flickering screen on transient failures)
module RemoteData.Refresh exposing (RefreshableRemoteData, isRefreshReady, refresh, refreshSend)

import Http
import RemoteData


type alias RefreshableRemoteData data =
    { current : RemoteData.WebData data
    , upcoming : RemoteData.WebData data
    }



-- are we already refreshing?


isRefreshReady : RefreshableRemoteData data -> Bool
isRefreshReady rrd =
    not (RemoteData.isLoading rrd.upcoming)



-- only update if the new response is not a failure when we have a previous success


refresh : RefreshableRemoteData data -> RemoteData.WebData data -> RefreshableRemoteData data
refresh rrd upcoming =
    case rrd.current of
        RemoteData.Success _ ->
            if not (RemoteData.isFailure upcoming) then
                { rrd | current = upcoming, upcoming = RemoteData.NotAsked }

            else
                { rrd | upcoming = RemoteData.NotAsked }

        _ ->
            { rrd | current = upcoming, upcoming = RemoteData.NotAsked }



-- make sure we're not overloading the backend, if query is already loading, skip scheduling a new one


refreshSend rrd msg call =
    if isRefreshReady rrd then
        ( { rrd | upcoming = RemoteData.Loading }, Http.send msg call )

    else
        ( rrd, Cmd.none )

@sectore
Copy link

sectore commented Feb 9, 2020

It might be worth to check out an answer by @raveclassic (author of remote-data-ts - very similar to remotedata, but written in TypeScript) to a similar question about adding a refreshing state to remote-data-ts :

All this questions led us to one simple answer - keep UI things in UI and don't pollute data model.

^ see devexperts/remote-data-ts#22 (comment)

@tgelu
Copy link

tgelu commented Jul 27, 2020

I went back and forth with trying to make a nice implementation of this, though my use case was regarding infinite scrolling where I needed two extra branches: | LoadingMore data and | FailedLoadMore error data, both in TS and Elm.

I think in Elm my preferred option is to build my own type that wraps RemoteData. I see no point in trying to complicate this simple type with something that does not represent the majority of use cases; or if it does, then it should definitely be a new type/package so that both the experience for simple and complex scenarios are optimal.

@pete-murphy
Copy link

pete-murphy commented Apr 11, 2024

I'd love to hear how people are getting on with the various solutions in this thread. What works and what doesn't work? In the interim, I'm adding a Refreshing constructor to my remote data type…

A solution that's been working well for me, and I don't think I've seen put forth in the thread so far, is collapsing the NotAsked and Loading states and pairing with a loading Bool.

module Api.Data exposing
    ( Data
    , Value(..)
    , notAsked
    , loading
    , succeed
    , fail
    -- ...
    )

type Data a
    = Data (Internals a)


type alias Internals a =
    { value : Value a
    , isLoading : Bool
    }


type Value a
    = Empty
    | HttpError Http.Error
    | Success a



-- CONSTRUCTORS


notAsked : Data a
notAsked =
    Data { value = Empty, isLoading = False }


loading : Data a
loading =
    Data { value = Empty, isLoading = True }


succeed : a -> Data a
succeed a =
    Data { value = Success a, isLoading = False }


fail : Http.Error -> Data a
fail error =
    Data { value = HttpError error, isLoading = False }

-- ...

Data is isomorphic to ExceptT Http.Error (MaybeT (WriterT Any)) in Haskell, which has a Monad instance that can be used to guide the andMap, andThen implementations. And transitioning to loading state is done by toLoading : Data a -> Data a.

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

No branches or pull requests