Skip to content

Instantly share code, notes, and snippets.

@stefan-wullems
Last active June 9, 2022 11:59
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save stefan-wullems/58fad5f5e715aae11f009901341c762b to your computer and use it in GitHub Desktop.
Save stefan-wullems/58fad5f5e715aae11f009901341c762b to your computer and use it in GitHub Desktop.

Caching models rethink

Now that we are not dependent on routeFromModel I was thinking: Maybe we should do a rethink of the way we store models in admin only.

Everything from every page is cached

I think caching is a performance/complexity tradeoff that requires reasoning and a conscious decision. Right now we have all the page models stored as Maybe SomeModel in a large record, we literally cache everything in every model for every page, even pages that have nothing we would like to cache.

I think we should rethink this and come up with an opt-in way of caching.

Boilerplate and complexity caused by the everything from every page is cached approach

  1. Whenever the url changes we have to check whether there is already a cached model for the page or not.
updatePage ... =
  case route of
    Route.SomePage ->
      case model.someModel of
        Nothing ->
          -- We don't have a cached model yet
          SomePage.init

        Just someModel ->
          -- We already have a model so 
          SomePage.updatePage someModel

This is first of all boilerplate that repeats over and over and over for every page, but second of all, it has some additional problems like this:

We have a model for the UserStatsPage and when we leave that page, we don't throw away that model. Now when we visit that page again for a diffent user, instead of reinitializing, we use the model from the previous user. To prevent this kind of stuff we have to put checks like this in the codebase.

updatePage ... =
  if model.uid /= uid then
        init embed uid route

  else
    ..actual update page..

This requires us to keep state of the user id.

  1. We have to use Maybe.map to update the page model.
 Cmd.pure { model | someModel = Maybe.map (SomePage.update msg_) model.someModel }

This is not a huge deal, but it's everywhere. It would definitely be cleaner if we were able to get rid of this.


Maybe there are some other places where this kind of caching makes things more tricky.

Cacheless

To get rid of caching page models completely, let's take a look at a sum type approach for page models.

The code could look like this

type Model 
  = FooPage FooPage.Model
  | BarPage BarPage.Model
  
init : (Msg -> msg) -> Route -> (Model, Cmd msg)
init embed  route =
   case route of
      Route.Foo fooParams ->
         FooPage.init (embed << FooMsg) fooParams
         
      Route.Bar ->
         BarPage.init (embed << BarMsg)
         
update : (Msg -> msg) -> Route -> Msg -> Model -> (Model, Cmd msg)
update embed route msg model =
   case (route, msg, model) of
     (Route.Foo fooParams, FooMsg fooMsg, FooPage fooModel) ->
        Tuple.mapFirst FooPage <| 
          FooPage.update (embed << FooMsg) fooParams fooMsg fooModel 
          
     (Route.Bar, BarMsg barMsg, BarPage barModel) ->
        Tuple.mapFirst BarPage <|
          BarPage.update (embed << BarMsg) barMsg barModel
          
     _ -> Cmd.pure model
          
updatePage : (Msg -> msg) -> Route -> Model -> (Model, Cmd msg)
updatePage embed route model =
  case (route, model) of
    (Route.Foo fooParams, FooPage fooModel) ->
       Tuple.mapFirst FooPage (FooPage.updatePage (embed << FooMsg) fooParams fooModel)
       
    (Route.Bar, BarPage barModel) ->
       Cmd.pure (BarPage barModel)
       
    _ ->
      -- The route doesn't match with the current page the model is on.
      init embed route
      
view : (Msg -> msg) -> Route -> Model -> Html msg
view embed route model =
  case (route, model) of
    (Route.Foo fooParams, FooPage fooModel) ->
       FooPage.view (embed << FooMsg) fooParams fooModel
       
    (Route.Bar, BarPage barModel) ->
       BarPage.view (embed << BarMsg) barModel
       
    _ -> Html.text "Something went wrong. Please contact the elm team!"

I think this would be an improvement to the readability of the code becaude there's a lot less boilerplate and the boilerplate that is there is simpler.

Explicit caching

To reintroduce the caching functionality, I think we should add a new kind of object called the Cache that only contains things we have consciously decided to cache. It's going to be an argument to init functions that explicitly say they want to try to derive something from the Cache and each page that wants to cache some of their model will define a new cache : Model -> Cache -> Cache function that will be derive the new cache from the model whenever the page is left.

All the details of the Cache I want to leave open, also the principles to decide what to cache and what not to cache need to be formed, but I think this would be a major simplification of the codebase.

@simvux
Copy link

simvux commented Jun 9, 2022

I remember pushing for getting rid of the Maybe SubModel with a separate Cache very early on, but I was never really able to come up with a proper alternative. So glad to see this being brought back up but in a much more formal way.

I'm not sure how much boilerplate we'd be saving though. Because having an explicit cache means we'd no longer be able to derive cache of the model that already exists. Meaning that we'll need to explicitly add the interoperability between the cache and the real models, which would be more than just the Maybe.withDefault Sub.init approach we've got.

Regardless; it'd definitely be a huge plus in terms of readability. Having the different contents that can be viewed on screen be represented by a sum type makes so much more sense from a logical point of view. And our current approach to caching brings down the readability of all existing code rather than being it's own monolith on the side (which I would prefer).

@kraklin
Copy link

kraklin commented Jun 9, 2022

I like the idea in general, I'm little bit worried about loosing performance for the "tabbed" detail view as we would need to load every tab when we would like to switch to it (and some can take quite a while for sure) :(

Regarding the explicit caching, I think that we would end up with something like Store pattern in the end and we will end up with RemoteData states instead of Maybe (or maybe Maybe will be enough there)

So I'm little bit torn apart here, I definitely agree that we don't need caching on every level - e.g. the top one. But it is quite handy to have it on the "Detail" level. Also I remember how I hated that we were hitting BE on each tab switch inside the Statistics page even though nothing could change in between those two.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment