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.
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.
- 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.
- 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.
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.
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.
I remember pushing for getting rid of the
Maybe SubModel
with a separateCache
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).