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

[Feature request] Make caching of compose configurable #381

Closed
Sayan751 opened this issue Jun 26, 2019 · 11 comments · Fixed by #382

Comments

@Sayan751
Copy link
Contributor

commented Jun 26, 2019

I'm submitting a feature request

  • Library Version:
    1.11.0

Please tell us about your environment:

  • Operating System:
    Windows 10

  • Node Version:
    10.15.0

  • NPM Version:
    6.4.1
  • JSPM OR Webpack AND Version
    webpack 4.35.0
  • Browser:
    all

  • Language:
    TypeScript 3.5.2

Current behavior:
The view-model and view used in compose is currently cached, that is whenever the model changes, the view-model and view is never recreated and just reactivated with the new model. This makes sense for most of the cases. However, this causes problem for some cases. For example, when validation is used in a custom element. When the model is changed, old validation rules still gets triggered. Some workarounds in this case can be to use an explicit signal behavior on compose to force recreation of VM, or to reset the validation rules somehow (I tried the later, but without success). For more details on the use case, please refer this.

Expected/desired behavior:

  • What is the expected behavior?
    Naive expectation is that the VM and View will be recreated, considering all custom elements are transient. However, in this case I would suggest for a @bindable cache:boolean flag which can be used in processChanges to force the recreation. This is similar to the cache property of if.bind.

  • What is the motivation / use case for changing the behavior?
    Any workaround will take considerable time on my part due to non-trivial refactoring, whereas the solution with the configurable cache looks very much easily doable.

If you are interested for a PR, I would like to give it a try.

@bigopon

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

@Sayan751 absolutely, that would be a nice addition.

cc @EisenbergEffect @fkleuver for reflection on vNext

@StrahilKazlachev

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

I don't agree it is appropriate to be called caching. If it ends up being called cache, or derivative, I would expect, when set to "cache: true", that a given ViewModel, View or pair(of the previous) would only be "created" once, regardless how many time the combination happens later on.
So if I:

  1. bind A for view-model
  2. then bind B for view-model
  3. and now I bind again A for view-model

I would expect that A is "created" only once, and the second time is using the "cache" - this is how if should behave, if I'm not wrong. And this is not the current behavior.

@Sayan751

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

I agree with your point.

My requirement matches more with the activationStrategy.replace, that replaces the routed VMs when redirected to the same VM. Maybe the new property needs to be called something like that. For the sake of keeping it similar I would suggest to simply call it activation-strategy. And then use a string enum with following values reactivate (mimics the default and current behavior), and replace (the view-model, and view are recreated on change). A different set of values are needed in this case, as the some of the values for the router activation strategy do not make much sense in this case.

What is your view on that?

@StrahilKazlachev

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

Then what about reusing replace and invoke-lifecycle? <compose> only has activate.
Though invoke-lifecycle may imply the same as cache 🤔

@Sayan751

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

@StrahilKazlachev The new enum can be created with those 2 values. invoke-lifecycle also makes sense in this case, as the compose practically invokes the activate hook which is a lifecycle hook. activate and invoke-lifecycle are pretty synonymous in this case. Unless it is weirdly odd third option, I am fine with either of these 2 (or an more appropriate one), plus documentation. I leave the final choice to you guys.

Sayan751 added a commit to Sayan751/templating-resources that referenced this issue Jun 26, 2019
feat(compose): @bindable activation-strategy
This commit introduces @bindable activation-strategy for compose.
The default strategy is 'invoke-lifecycle', which is also the current
strategy. The strategy 'replace' forces re-creation of bound view and
view-model on model change. This is helpful, where the re-instantiation
of view and view-model is needed on model change. For example, when
validation is used in a custom element; on model change, old validation
rules still gets triggered if the view-model is not reinstantiated.

Fixes aurelia#381
@fkleuver

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

I'm not so sure if using the lifecycle strategy enum for compose is a good idea. It really shouldn't even have been called activate in the first place because it has absolutely nothing to do with routing.
Do we really want to double down on a potential source of confusion like that?

@Sayan751

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

@fkleuver The only lifecycle hook, in bound VM, called from compose is "activate". Considering that the activation-strategy is relatable in this context. As the activationStrategy.replace provides similar functionality for routed view in similar use case, it also seems to be easily understandable. For this purpose, I think it is not bad going with the "activation-strategy" concept.

@StrahilKazlachev

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

@fkleuver actually the async hooks canActivate/activate/... are already used in different combinations in different places - router, composition engine, <compose>, dialog, in vCurent ¯_(ツ)_/¯

@Sayan751

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2019

I tested the change on my project (so it's officially "works-on-machine" certified 😛). Once the terminologies are settled, I can update documentation and create 2 PRs in both the repositories.

@Sayan751

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2019

Another idea is to call this something like adaptation-strategy or change-strategy, since this affects how the changes will be handled by compose. With this the former 2 enum values (replace, and invoke-lifecycle) can still be used.

@fkleuver

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

@StrahilKazlachev And it's always been a massive source of confusion when I was still learning Aurelia. I think compose, dialog etc sharing the same lifecycle name for the purpose of "loading" and "unloading" is fine.
They should just all be different from the router lifecycle names because they are so very different in terms of timing and signature.

Sayan751 added a commit to Sayan751/templating-resources that referenced this issue Jul 3, 2019
feat(compose): @bindable activation-strategy
This commit introduces @bindable activation-strategy for compose.
The default strategy is 'invoke-lifecycle', which is also the current
strategy. The strategy 'replace' forces re-creation of bound view and
view-model on model change. This is helpful, where the re-instantiation
of view and view-model is needed on model change. For example, when
validation is used in a custom element; on model change, old validation
rules still gets triggered if the view-model is not reinstantiated.

Fixes aurelia#381
Sayan751 added a commit to Sayan751/templating-resources that referenced this issue Jul 4, 2019
feat(compose): activation strategy hook in VM
This commit adds the support to determine the activation strategy of the
bound view-model by invoking the determineActivationStrategy hook in the
view-model, when implemented. The activation strategy from the
view-model always overrides the one in the instance of `compose`.

Addendum for aurelia#381
Sayan751 added a commit to Sayan751/templating-resources that referenced this issue Jul 10, 2019
chore(compose): post review changes
- Refactored activation strategy implmentation
- Added negative tests
- Minor changes in docs

Issue aurelia#381
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.