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

feat(cachedComputedFrom): decorator for computed properties with caching #398

Closed
wants to merge 5 commits into from
Closed

feat(cachedComputedFrom): decorator for computed properties with caching #398

wants to merge 5 commits into from

Conversation

doodlesbykumbi
Copy link

@doodlesbykumbi doodlesbykumbi commented May 7, 2016

cachedComputedFrom is just a caching-enabled version of computedFrom.
cachedComputedFrom implements temporary caching for repeated fast access to the computed property while it's dependencies remain unchanged.

I did some profiling and cachedComputedFrom was more performant where it needed to be, i.e. when the the computed property is the same if it's dependencies remain unchanged.

…ements temporary caching for repeated fast access while dependencies remain unchanged
@doodlesbykumbi doodlesbykumbi changed the title cachedComputedFrom decorator cachedComputedFrom decorator for computed property the remain unchanged when their dependencies are unchanged May 7, 2016
@doodlesbykumbi doodlesbykumbi changed the title cachedComputedFrom decorator for computed property the remain unchanged when their dependencies are unchanged cachedComputedFrom decorator for computed properties that remain unchanged when their dependencies are unchanged May 7, 2016
@doodlesbykumbi doodlesbykumbi changed the title cachedComputedFrom decorator for computed properties that remain unchanged when their dependencies are unchanged feat(cachedComputedFrom): decorator for computed properties that remain unchanged when their dependencies are unchanged May 7, 2016
@doodlesbykumbi doodlesbykumbi changed the title feat(cachedComputedFrom): decorator for computed properties that remain unchanged when their dependencies are unchanged feat(cachedComputedFrom): decorator for computed properties with caching May 7, 2016
@AshleyGrant
Copy link
Collaborator

@jdanyow what are your thoughts on this PR?

@EisenbergEffect
Copy link
Contributor

Perhaps something like this should be the default behavior?

@doodlesbykumbi
Copy link
Author

@EisenbergEffect Possibly.

Initially, I thought of augmenting the computeFrom decorator to have a toggle for caching but I don't think that's a good idea just because it's giving one thing too much to do.

I think there should definitely be 2 different behaviours, since it's not clear the sort of functions people use as their getters, they could well be impure and be expected to cause a myriad of side effects every time they are invoked. It's always good to have a choice

@brandonseydel
Copy link
Member

What about just making the default to not cache unless specified. (I would like it to be the other way, but as you stated people could currently be doing some additional logic in their getters)

@computedFrom("") -> not cached
@computedFrom("", true) -> cached

@EisenbergEffect
Copy link
Contributor

@jdanyow What if we aliased computedFrom to be computed and then we added the cached mechanism using a decorator called pure?

@brandonseydel
Copy link
Member

brandonseydel commented May 9, 2016

What if you want to specify a completely different object for the caching mechanism?

@computedFrom("firstName", "lastName")
@cached -> original proposed implementation
@cached("cachingMechanism")
get fullName(){//returns concat};

Analyzes a different object to determine whether or not the value needs to be refreshed.
(maybe first name has changed but you don't want to propogate the changes until last name has also changed)

@doodlesbykumbi
Copy link
Author

@EisenbergEffect hey, so whatever you guys decide is best I'd be more than happy to implement it. The intention of this pull request really was both to get feedback on my solution to a problem I faced while creating an aurelia app but also to get my foot in the door with regard to making a habit of contributing to aurelia. It's not clear to me what needs contribution but I hope this is the beginning.

@EisenbergEffect
Copy link
Contributor

Awesome! Thanks for being so willing to join in and help. @jdanyow Do you have any thoughts on this?

@jdanyow
Copy link
Contributor

jdanyow commented May 15, 2016

Definitely a solid idea. I think this would make a nice plugin. Couple thoughts/suggestions:

  1. Using string.split to parse the expression is probably ok for 99% of the real-world scenarios. computedFrom supports any expression that works in binding (eg foo[0].bar.baz)- not sure if that's something you wanted to support as well.
  2. You could exchange the .every and .map for simple for-loops to avoid extra closures
  3. I'm wondering if it would be cleaner/more-performant to put the caching logic in the ExpressionObserver instead. There may be some logic still needed in the decorator to check for the presence of the observer when the property is being accessed and forward the call to the observer's getValue method (assuming it has caching).

@jdanyow jdanyow closed this May 15, 2016
@doodlesbykumbi doodlesbykumbi deleted the cachedComputedFrom branch February 16, 2018 01:22
@Truffula
Copy link

Truffula commented Jun 4, 2020

Did this ever ended up being implemented anywhere (as a plugin or otherwise)?

@bigopon
Copy link
Member

bigopon commented Jun 4, 2020

@Truffula thanks for notifying about this. I didn't know we had a PR for this. The current state of this plugin does not guarantee it works in all scenarios, as dependencies are simply retrieved via index/keyed operator [].

There's a plugin here that gives you the cached computedFrom behavior: https://github.com/bigopon/aurelia-deep-computed (I created this)

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.

7 participants