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

WIP: caching #425

Closed
wants to merge 4 commits into from
Closed

WIP: caching #425

wants to merge 4 commits into from

Conversation

jdanyow
Copy link
Contributor

@jdanyow jdanyow commented Oct 17, 2016

@jdanyow
Copy link
Contributor Author

jdanyow commented Oct 17, 2016

@EisenbergEffect, @bryanrsmith - here's a first pass at adding controller caching to the router/viewport

Changes:

  • New optional property on the route config: cacheable?: boolean
  • The RouterView takes a dependency on a new class, the ViewPortCache
  • RouterView attempts to retrieve the controller from the ViewPortCache when processing a viewport instruction.
  • If the RouterView is forced to create a controller because of a cache miss, it will push the controller into the cache.
  • The router's route-loading pipeline step attempts to retrieve the "activatable" (the viewmodel) from the ViewPortCache. A cache miss will result in the standard viewmodel instantiation process.
  • The ViewPortCache uses the combination of viewport name, fragment and querystring as the cache-key.
  • The RouterView clears it's ViewPortCache when it unbinds. Clearing the cache removes and unbinds all controllers in it's cache. This ensures memory leaks are not introduced by the caching.

Non-obvious things worth additional thought/review:

  • Some additional coupling has been introduced between the router and the viewport (RouterView). The viewport is assumed to have a cache property that implements the ViewPortCache interface.
  • The entire lifecycle (activation and composition) is preserved. I'm thinking a cached controller probably shouldn't be unbound and re-bound though. Preventing the unbind when removing from the slot will require additional work. The activation lifecycle is preserved, but cached controllers are not unbound after they are detached from the viewslot, likewise they are not re-bound when they are re-attached to the viewslot.

Still working on:

  • bugs related to child-routers
  • tests
  • need to consider route pre-loading (related feature request) in the design.

Let me know what you think!

@EisenbergEffect
Copy link
Contributor

EisenbergEffect commented Oct 17, 2016

The description makes sense to me. I'll try to review the code at a deeper level this week. @jdanyow We should try to get more eyes on this. Can you track down the other issue related to this and reference those people in? @cmichaelgraham Maybe you can take a look at this as well and see if it will handle your map rendering scenario.

@jdanyow
Copy link
Contributor Author

jdanyow commented Oct 17, 2016

sure- here's some people from #173: @fopsdev, @jods4, @stefan505, @dpinart, @Scapal, @heruan, @Vaccano

and from aurelia/templating-binding#95: @dkent600

@EisenbergEffect
Copy link
Contributor

Perfect. I'll do a technical review but hopefully the community members above can help validate that the real use cases are handled by this implementation.


@transient()
export class ViewPortCache {
controllers = {};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember we had to ensure correct cache lookup by
controllers = Object.create(null);
I guess it's should be applied here also..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stoffeastrom - I can make that change. The issue solved by Object.create(null) will not be an issue here due to the format of the keys but it's good to be consistent.

@dkent600
Copy link
Contributor

Will this caching be applied to everything contained in a view, including custom elements, manually-composed views, sub-routes, etc.?

How will this interact with or be related to container options like singleton, transient, etc. that can be applied to a view model?

@jdanyow
Copy link
Contributor Author

jdanyow commented Oct 17, 2016

@dkent600 good questions, it works like this:

When a route is activated, the resulting view and view-model are cached. Subsequent activations of the same route (same fragment and querystring) will reuse the view and view-model instance. The view and view-model instance will not be unbound.

The change in behavior requires opt-in via a property in the route config and does not interfere with resolvers like singleton, transient, etc.

@dkent600
Copy link
Contributor

dkent600 commented Oct 26, 2016

Will this play well with aurelia-validation (or vice-versa)? In other words, will cached views retain all of their validation state and remain independent of other views as if they were uncached?

@jdanyow
Copy link
Contributor Author

jdanyow commented Oct 26, 2016

@dkent600 yep- unbind is what causes the validation errors, etc to unrender. Cached view+viewmodel pairs are not unbound, so the validation state will be preserved along with the rest of the state.

@dkent600
Copy link
Contributor

Can't wait for this to be released!

@mikael-arvidsson
Copy link

Amazing! Can't wait! @jdanyow keep up the good work!

@jods4
Copy link
Contributor

jods4 commented Nov 11, 2016

The ViewPortCache uses the combination of viewport name, fragment and querystring as the cache-key.

Should that be configurable? Like: maybe I want to reuse the same view whatever the query string is, and it might be taken into account during lifecycle events, most likely activate? This could even be true of the fragment if it contains parameters like an id?

Is there a risk that the cache grows unboundedly?

@jods4
Copy link
Contributor

jods4 commented Nov 11, 2016

Side-note: if I understand correctly how this works, good communication/doc will be needed so that people understand clearly the difference in controls between attached/detached and bind/unbind.

Currently, when wrapping 3rd party library controls like jQuery plugins, I see people do their $(element).magicButton() and $magicButton.destroy() in either of those lifecycle event pairs. And honestly, it often doesn't matter much.

With the new view caching infrastructure, it is important to understand that unbind won't be called but detached will. For example, if you want your jQuery plugin to persist some internal state when going back and forth between views, you must destroy it in unbind.

Integrating a JS control with internal state (e.g. a Map viewer that has position and zoom) and showing how said state is preserved when switching views would be a nice proof of concept for view caching.

export class ViewPortCache {
controllers = Object.create(null);

get(viewPortName, viewPortConfig, navigationInstruction) {
Copy link
Contributor

@gheoan gheoan Nov 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should viewPortConfig be the first argument, as the others can be ommited if viewPortConfig.cacheable is false.

@EisenbergEffect
Copy link
Contributor

@jods4 Great feedback!

@gregorydickson
Copy link

+1 on this feature and many props to @jdanyow and @jods4 for the contribution.

@mikael-arvidsson
Copy link

What's the status of this? My team is waiting impatiently! Really want this feature!

@jdanyow
Copy link
Contributor Author

jdanyow commented Dec 21, 2016

@jods4 - I just wanted to reply to some of your questions from November 10th that I hadn't responded to:


The ViewPortCache uses the combination of viewport name, fragment and querystring as the cache-key.

Should that be configurable? Like: maybe I want to reuse the same view whatever the query string is, and it might be taken into account during lifecycle events, most likely activate? This could even be true of the fragment if it contains parameters like an id?

The ViewPortCache has a createKey function that can be overridden to supply custom logic:

import {ViewPortCache} from 'aurelia-router';

ViewPortCache.prototype.createKey = function(viewPortName, navigationInstruction): string {
  const { fragment, queryString } = navigationInstruction;
  return `ViewPort: '${viewPortName}'; Fragment: '${fragment}'; QueryString: '${queryString}';'`;
}

Is there a risk that the cache grows unboundedly?

Yes and no- the caching will occur if the route's config has cacheable: true. If you were to make a route like { route: 'users', moduleId: './users/users-list', title: 'Users' } cacheable, you won't run into issues. Making something like { route: 'users/:id', moduleId: './users/user-detail', title: 'User' } cacheable would probably be a bad move.

@tkallsen
Copy link

Is there a risk that the cache grows unboundedly?

Will there be a way to clear the cache (selectively/everything)? So that you can insert some intelligence in what stays cached and what should be ejected from the cache depending on user actions.

@jods4
Copy link
Contributor

jods4 commented Dec 21, 2016

The ViewPortCache has a createKey function that can be overridden to supply custom logic:

What I don't quite like with this design is that there is a central point where the rules for all the routes/views have to be coded.
It would feel better if this logic could somehow be embedded into each view.

Making something like { route: 'users/:id', moduleId: './users/user-detail', title: 'User' } cacheable would probably be a bad move.

That's what I was concerned about. I think this is a core scenario so:

  • It would be nice to have a pit of success and not of failure. As it stands, this is a huge pitfall.
  • The case might be common enough that we want to include smarts about route parameters? Like by default views are cached regardless of parameters and lifecycle events execute if a parameter changed?

I have not thought deeply about any of this, so I don't have any solution to propose. I'm just sayin'.
¯\_(ツ)_/¯

@dkent600
Copy link
Contributor

dkent600 commented Dec 21, 2016

What I don't quite like with this design is that there is a central point where the rules for all the routes/views have to be coded.
It would feel better if this logic could somehow be embedded into each view.

My two cents: This proposed design (embedding caching config in a view) has the feel of angular to me. OK, not necessarily a bad thing, being like angular, but it seems to me like it would be making the view responsible for something not in its purvue. Ie, not one of the view's appropriate responsibilities. ("How does one navigate to a view" being outside of how a view does what it does.)

As such, it would be stealing responsibility and knowledge from the routing configuration where all other knowledge about routes and navigation is currently contained.

@jods4
Copy link
Contributor

jods4 commented Dec 21, 2016

@dkent600 I don't know about Angular so there was no influence.

I don't like spreading concerns all over the place, where it's hard to discover:

  • If a junior dev starts a new view, I think it's better that it can look at an existing view and see everything it should implement. The more magic strings referencing this view exists in other files (the router def, the view cache def) the more likely he will miss that.
  • Likewise when refactoring / moving / deleting code.
  • I like seeing all the info in one place. When I open the code of a view, I like seeing whether it's being cached or not, with which parameters, without having to dig up random files in a totally different place.
  • Having central config means more contention in team work, possible merge conflicts, etc.

it would be making the view responsible for something not in its purvue

It is not clear to me that deciding whether a view supports/needs to be cached, what parameterization is required, whether its lifecycle must be called again or not is the view's responsibility or the caching infrastructure's. I would argue the former.

As you can guess, I'm a little annoyed by the router as well and thinking of setting up some decorator-based routes configuration. ;)

@jdanyow
Copy link
Contributor Author

jdanyow commented Dec 28, 2016

There's a couple of primary scenarios this PR aims to solve:

  1. You open a record, the record has a several sub-sections manifested by a child router, chid router-view and a series of tabs or pills bound to the child router's nav model. Navigating back and forth between tabs requires caching to improve ux. With the current design, all you need to do is put cachable: true on child routes. Once a child route is activated it will remain in the cache (which is owned by the child router-view) until the child router-view is destroyed (eg the main record that owns the child router / child router-view is deactivated).

  2. An expensive screen should be cached for the lifetime of the application. For example, there's a route that displays a map. When you navigate away from the map route, you want the map's state (zoom level, layers, position, etc) preserved for when the map route is re-activated. Again, with the current design, putting cachable: true on the route is sufficient.

I'm sure there are many other scenarios however these are the ones I most commonly encountered in the discussions I've had and issues I've reviewed. I think we could add an option of putting the caching decision logic in the view-model to improve flexibility.

@jods4
Copy link
Contributor

jods4 commented Dec 28, 2016

Those really stand out as main use cases for me. Especially number 2. Not that the screen is necessarily "expensive" but might contains state outside of VM (like your example: inside a JS map control) and you want to preserve that state when navigating back.

That said, I also think that good API design needs to think of what some users will invariably try to do and at least avoid obviously bad outcome. cache: true in the router is good enough for me, but I am still worried by the behavior for parameterized routes, in particular the memory leaks that will result.

To avoid complexity and ship something, maybe in a first approach we could say that caching is done by route name only?

@dkent600 mentions Angular, what is their approach to the memory leak problem?

@stephenstroud
Copy link

stephenstroud commented Apr 6, 2017

Is there an update for this feature, thanks? @jods4 @jdanyow @dkent600 @gheoan

@RomainLanz
Copy link

Guys ? @jods4 @jdanyow @dkent600 @gheoan @EisenbergEffect

c/c @jwx @Darkless012

@xenod
Copy link

xenod commented Jul 15, 2017

Any updates? This is still the no #1 requested feature from me, my team and our clients!

@RicoSuter
Copy link

Same here +1

@stephenstroud
Copy link

stephenstroud commented Jul 15, 2017 via email

@alu
Copy link

alu commented Nov 16, 2017

@jdanyow @EisenbergEffect

Any updates? I want news from anything.

@EisenbergEffect
Copy link
Contributor

This feature was originally being funded by a company who needed it. However, they decided that they wanted us to focus our efforts elsewhere, so the work on this petered out. @jdanyow may be able to provide some information on what work is still required to bring this to production quality. It's a tricky problem to do this in a generic way. I'm not sure if Jeremy was able to address those issues or if there were still open questions.

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

Successfully merging this pull request may close these issues.

View and ViewModel caching option in router