Evaluated computed property dependencies first. Fixes #5143 #5145
Conversation
kevinpschaaf
requested review from
dfreedm,
sorvell and
TimvdLippe
as code owners
| * @return {boolean} true when the property is dirty | ||
| * @private | ||
| */ | ||
| function propertyIsDirty(inst, trigger, changedProps) { |
There was a problem hiding this comment.
I would put these functions before runComputedEffect. That makes reading these functions a bit easier, since runComputedEffect calls into these functions and I was confused which ones they were.
| let deps = info.computedDependencies; | ||
| if (!deps) { | ||
| deps = info.computedDependencies = []; | ||
| info.args.forEach(arg => { |
There was a problem hiding this comment.
I think this can be a info.args.reduce instead
| } | ||
| } | ||
| } | ||
|
|
||
| const TRANSITIVE_DEPENDENCY = '~transitive~dependency~'; |
There was a problem hiding this comment.
Can we use a Symbol here to make sure we are always unique? (however unlikely someone names a property exactly this)
| @@ -82,20 +84,23 @@ | |||
| * | |||
| * @param {Object} model Prototype or instance | |||
| * @param {string} type Property effect type | |||
| * @param {boolean=} cloneArrays Clone any arrays mapped in the map | |||
There was a problem hiding this comment.
Instead of adding a boolean parameter, I would rather see an Object{cloneArrays:boolean} per #4864
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This issue has been automatically closed after being marked stale. If you're still facing this problem with the above solution, please comment and we'll reopen! |
Updates the implementation of
runComputedEffectto recursively walk the graph of dependencies that are themselves computed and re-compute them if needed, which ensures computed properties are not computed out of order.The initial walk is still kicked off by the normal
runEffectsloop over changed properties, which ensures that no work related to computed properties occurs unless at least one dependency to a computed property has changed. An alternate implementation of running every computed effect each flush was considered, but it seems worth keeping the "pay-for-play" model intact. Thus, the effect de-duping mechanism still exists, which prevents a property later in the batch of changes from causing any computed effect that has already run during this flush from running again, and also prevents iloops for circular dependencies.When walking the graph of computed dependencies to a computed property, we track when a computed effect is being run transitively (vs. directly due to a dependency change in
changedProps) by passing a sentinelTRANSITIVE_DEPENDENCYproperty name torunComputedEffect'spropertyargument (which normally is the input dependency which triggered the change). This tracking is useful because transitively run computed effects need to dirty-check their dependencies, while effects that were directly run byrunEffectscan skip the expensive argument dirty check to occur since they are already guaranteed to have at least one argument dirty.Reference Issue
Fixes #5143