ViewFactory does not invalidate cache after the factories template changed

itfourp commented 6 years ago

I'm submitting a bug report

Library Version: 1.7.0

Please tell us about your environment:

Operating System: Windows 10
Node Version: 6.10.2
NPM Version: 5.3.0
JSPM OR Webpack AND Version JSPM 0.16.53
Browser: all
Language: all

Current behavior: Using custom elements in multiple repeats with view-cache enabled causes weird behaviour when different templates are used for each repeat and items are moved from one repeat to the other one. I guess the fragment of the cached view is not updated when the element was moved from one repeat to another.

This bug first occurred when I was using the oribella-aurelia-sortable library that uses a repeat with enabled cache to ensure touch-support is working correctly (see furthermore here and here).

Expected/desired behavior: For an example for both expected and current behaviour and how to reproduce please see here: https://gist.run/?id=2cd8a03e16f92ce64308f9ec841c3b70

What is the expected behavior? We could invalidate the cache when the template of the ViewFactory has changed so the ViewFactory is forced to re-create the view, but I am not sure if the will break touch-support yet again.
What is the motivation / use case for changing the behavior? To ensure the oribella-aurelia-sortable library can be used with touch support.

Alexander-Taran commented 6 years ago

@EisenbergEffect & @jdanyow It is probably a rare use case, but please have a look at this and the linked issue in aurelia-sortable plugin, when you will have some time.

EisenbergEffect commented 6 years ago

I think the issue is that the view caching mechanism isn't taking part overrides into consideration. @bigopon Interested in an interesting challenge?

@itfourp If you don't use the cache mechanism, does that cause you perf issues?

itfourp commented 6 years ago

@EisenbergEffect I don't think so, but since I need touch support and there is an issue with touch listeners and removed html elements I'm pretty much depending on the caching mechanism. I tried resolving the problem in the aurelia-sortable library itself, but that's pretty hackish and we decided to look for a better solution first.

stoffeastrom commented 6 years ago

@bigopon @EisenbergEffect Any updates regarding this?

As @itfourp said if you need touch support this needs to be fixed. Do we know where in the view cache mechanism we need to fix this?

EisenbergEffect commented 6 years ago

I don't think anyone has had a chance to look into this deeply yet.

bigopon commented 6 years ago

Because the issue is in the repaceable part, which is nested inside repeated element, it's quite difficult to work out. Maybe we can fix getCachedView() to give it some extra arguments so it can determine if the cache view is going to be correct

bigopon commented 6 years ago

@EisenbergEffect What about we introduces a special attribute similar to view-cache, say separate-cache so each Repeat will be associated with different cache ?

  <div repeat.for='item of items' separate-cache>
    ...
  </div>

EisenbergEffect commented 6 years ago

I'd be willing to entertain this idea. It is an edge case, so I'm not sure it's worth it to re-write the entire system. But if we can enable a simple setting like this, then the few people who really need this can opt in.

EisenbergEffect commented 6 years ago

Maybe it should be a named cache?

bigopon commented 6 years ago

It's not a rewrite, I think it can simply be some extra arguments around caching and retrieving cache in view factory

EisenbergEffect commented 6 years ago

That seems ok. Do you want to have a go at it?

bigopon commented 6 years ago

I'll give it a shot after the current feature set

itfourp commented 6 years ago

I'm very happy you've already got some ideas to fix this issue. If you're interested I could set up another gist or a github project that makes use of touchy-stuff and you could use to test your solution on.

bigopon commented 6 years ago

@itfourp No rush and that'd be great 👍

itfourp commented 5 years ago

Sorry, I completely forgot to mention I'm done with the setup of the touch-demonstration project for this issue. Please see here. If anything does not work as expected feel free to tell me.

bigopon commented 5 years ago

@itfourp Nice. Thanks!

bigopon commented 5 years ago

@itfourp I'm unable to reproduce the issue as you can see from this demo https://dist-ouuggzgmue.now.sh/ I'm not sure where the differences are, but I followed the step you described.

itfourp commented 5 years ago

@bigopon I am sorry my instructions have been unclear. In this example I've already set the view-cache to 0 in the sort-container.html to show the touch issues that occur with disabled view caching. To reproduce the behavior I reported here please just re-enable the caching.

itfourp commented 5 years ago

@bigopon I've updated my example repository here. There now are two views showing the problem with caching and without - hope it helps.

bigopon commented 5 years ago

Thanks @itfourp , I haven't got around to fix the issue, but I'll try to summarize the issue first for some discussion, before starting that work.

The normal usage of `repeat` + `view-cache` combo that doesn't cause issue:

Supposed we have the following as template of custom element named foo:

<!-- foo.html -->
<template>
  <div repeat.for="item of items" view-cache="*">
    ... bunch of content
  </div>
</template>

In this case, the template of repeat template controller doesn't let any foreign content leaked into it. So when repeat caches views in and retrieves views out, there is no issue.

The problematic usage of `repeat` + `view-cache` combo that causes the issue:

Suppose we have the following as template of a custom element named foo:

<!-- foo.html -->
<template>
  <div repeat.for="item of items" view-cache="*">
    <div replaceable part="part-1">

    </div>
  </div>
</template>

And it will be used in 2 places inside app root like following:

<template>
  <foo>
    <template replace-part="part-1">
      ... bunch of content 1
    </template>
  </foo>
  ... bunch of content
  <foo>
    <template replace-part="part-1">
      ... bunch of content 2
    </template>
  </foo>
</template>

What will happen at runtime is:

both usage of <foo> will cause <div replaceable part="part-1"> to be replaced with either <template replace-part="part-1"> content 1 or <template replace-part="part-1"> content 2. And this is permanent replacement.
when repeat caches in, it caches views that have nested content that has been replace by either <template replace-part="part-1"> content 1 or <template replace-part="part-1"> content 2, without trying to distinguish them.
when these caches are retrieved, depends on the order, replace-part="part-1" > content 1 may get the view that was from replace-part="part-1"> content 2 and vice versa.

This issue comes from the fact that no matter how many instances of repeat there are, they all share the same underlying ViewFactory instance. So to fix this bug, we need to have a way to tell the repeat that it should not use the default cache, via some attribute (API is for discussion):

<!-- foo.html -->
<template>
  <div repeat.for="item of items" view-cache="*" separate-cache-for-each-repeat>
    <div replaceable part="part-1">

    </div>
  </div>
</template>

This requires new APIs for ViewFactory to enable the ability to return views to different caches set. I think this is a simple extension, as by default every view will just return to default cache, unless explicitly told what cache it should go to.

thoughts? @EisenbergEffect @fkleuver @itfourp @stoffeastrom @Alexander-Taran

EisenbergEffect commented 5 years ago

I'd like @fkleuver's thoughts on this, particularly as it pertains to vNext. If possible I think we should endeavor to solve this for vNext and backport to vCurrent so that our solution here is forward-compatible and so that this usage scenario is captured and handled going forward (no regression in vNext).

I'm not particularly fond of the API above 😏 So, some further design thinking on that would be nice. Ideally, the option wouldn't need to be specified at all. We'd just detect that there's a replaceable part involved and automatically create a named cache to handle this behind the scenes.

fkleuver commented 5 years ago

For a sortable/draggable repeater in vNext, you would use keyed mode. This reorders the elements instead of re-binding them and thereby retains the element state (event listeners, css, etc). That simplifies this situation a lot.

For caching views in keyed mode, my immediate thought is: KeyedViewFactory. Repeater passes the key to factory.create, and factory will only retrieve a cached view if one already exists for that one key. This inherently accounts for anything going back-and-forth between different repeaters (a common use case; dragging elements from one list to another).

Regarding the edge case with nested replaceable, this problem might not exist in vNext in the first place. Detaching only happens for root elements, so nested views simply stay in the DOM of their (cached) parent which, when un-cached, is appended back to the DOM as one big whole again.

To make all of this work in vCurrent I do see the possibility of backporting keyed mode (this might even be easier to do in vCurrent than it was in vNext because there is less lifecycle order/timing stuff to account for) as well as having a KeyedViewFactory. For the replaceable problem we would just have to experiment a little with various ways to propagate the key of a parent down to child views so they're locked together as a set. I think this would apply to any nested views, not just replaceable

Without keyed mode, I'm not sure if we should even try. This is the primary use case for keyed mode. That inherently brings with it a key. I think that takes away a fair amount of design uncertainties.

@bigopon maybe either you or me should port @itfourp 's repro to vNext and see if it already works or not. In any case vNext is much closer to being able to deal with this already than vCurrent is, so we should look there first imo.

bigopon commented 5 years ago

@bigopon maybe either you or me should port @itfourp 's repro to vNext and see if it already works or not. In any case vNext is much closer to being able to deal with this already than vCurrent is, so we should look there first imo.

I'll do this soon.

deap82 commented 2 years ago

I think I've stumbled upon something related to this issue, but for now managed to work around it. Just wanted to know if there has been anything done about this that perhaps I could try, before elaborating more on my specific case (which involves both sorting [through a value converter] and replaceable parts in a probably pretty advanced way... 😬). In short, the issue is with checked bindings for a set of radiobuttons inside one of those replaceable parts, in each of the sorted items).

ping @bigopon

aurelia / templating