-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
prevent cyclical store computations, and computation duplication #964
Conversation
store.js
Outdated
var visited = blankObject(); | ||
|
||
function visit(key) { | ||
if (cycles[key]) { | ||
throw new Error(`Cyclical dependency detected — a computed property cannot indirectly depend on itself`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably shouldn't use template literals in code that's going to get shipped to users.
It's also bugging me a bit having this whole error message text end up in production code, but I don't see a reasonable way around that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, good catch. Should perhaps add a test for ES6+isms. I agree about the error message. Maybe we could just shorten it? e.g. just 'Cyclical dependency detected'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Using a shorter error message seems like a reasonable compromise.
Codecov Report
@@ Coverage Diff @@
## master #964 +/- ##
=======================================
Coverage 91.76% 91.76%
=======================================
Files 109 109
Lines 4054 4054
Branches 1301 1301
=======================================
Hits 3720 3720
Misses 150 150
Partials 184 184 Continue to review full report at Codecov.
|
Something @adamhaile spotted yesterday (thanks for the eagle eye!) — when
Store
re-sorts its computed properties, it doesn't bother to mark which ones it's already seen, so in a situation like this......where both
c
andd
depend on a computed value, it would end up thinking it had as many as 5 things to recompute on each state change, rather than a maximum of 3.I also added some cyclical dependency detection, so that this...
...results in a helpful 'Cyclical dependency detected — a computed property cannot indirectly depend on itself' error, rather than a stack overflow.