From 4d8090aa158bc369fc679baccb29d7e843bd5ce6 Mon Sep 17 00:00:00 2001 From: Brian Takita Date: Tue, 1 May 2018 21:20:05 -0400 Subject: [PATCH 1/6] Fix https://github.com/sveltejs/svelte/issues/1399 Store - Cyclical Dependency Detected when child computed property defined before parent & grand-parent computed proprety --- store.js | 6 ++--- .../store-computed-compute-graph/_config.js | 22 +++++++++++++++++++ .../store-computed-compute-graph/main.html | 0 3 files changed, 25 insertions(+), 3 deletions(-) create mode 100644 test/runtime/samples/store-computed-compute-graph/_config.js create mode 100644 test/runtime/samples/store-computed-compute-graph/main.html diff --git a/store.js b/store.js index 42f17c9bb8..678c73b1b3 100644 --- a/store.js +++ b/store.js @@ -53,19 +53,19 @@ assign(Store.prototype, { var visited = blankObject(); function visit(key) { + if (visited[key]) return; + if (cycles[key]) { throw new Error('Cyclical dependency detected'); } - if (visited[key]) return; - visited[key] = true; - var c = computed[key]; if (c) { cycles[key] = true; c.deps.forEach(visit); sorted.push(c); + visited[key] = true; } } diff --git a/test/runtime/samples/store-computed-compute-graph/_config.js b/test/runtime/samples/store-computed-compute-graph/_config.js new file mode 100644 index 0000000000..ba7ac02f8d --- /dev/null +++ b/test/runtime/samples/store-computed-compute-graph/_config.js @@ -0,0 +1,22 @@ +import { Store } from '../../../../store.js'; + +const store = new Store(); + +export default { + store, + + test(assert, component, target) { + store.compute('dep4', ['dep1', 'dep2', 'dep3'], (...args) => ['dep4'].concat(...args)); + store.compute('dep1', ['source'], (...args) => ['dep1'].concat(...args)); + store.compute('dep2', ['dep1'], (...args) => ['dep2'].concat(...args)); + store.compute('dep3', ['dep1', 'dep2'], (...args) => ['dep3'].concat(...args)); + store.set({source: 'source'}); + assert.equal(JSON.stringify(store.get().dep4), JSON.stringify([ + 'dep4', + 'dep1', 'source', + 'dep2', 'dep1', 'source', + 'dep3', 'dep1', 'source', + 'dep2', 'dep1', 'source' + ])); + } +}; diff --git a/test/runtime/samples/store-computed-compute-graph/main.html b/test/runtime/samples/store-computed-compute-graph/main.html new file mode 100644 index 0000000000..e69de29bb2 From 058e1bddf394a38cb3c4e60452450b5521d42d56 Mon Sep 17 00:00:00 2001 From: Brian Takita Date: Tue, 1 May 2018 21:28:37 -0400 Subject: [PATCH 2/6] Added key & rootKey into 'Cyclical dependency detected' error message --- store.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/store.js b/store.js index 678c73b1b3..abc2f1886f 100644 --- a/store.js +++ b/store.js @@ -52,18 +52,20 @@ assign(Store.prototype, { var cycles; var visited = blankObject(); - function visit(key) { + function visit(key, rootKey) { if (visited[key]) return; if (cycles[key]) { - throw new Error('Cyclical dependency detected'); + throw new Error('Cyclical dependency detected. key: ' + key + ' rootKey: ' + rootKey); } var c = computed[key]; if (c) { cycles[key] = true; - c.deps.forEach(visit); + c.deps.forEach(function(dep) { + visit(dep, rootKey) + }); sorted.push(c); visited[key] = true; } @@ -71,7 +73,7 @@ assign(Store.prototype, { for (var key in this._computed) { cycles = blankObject(); - visit(key); + visit(key, key); } }, From 9d6d99686664fcac2391f0fd62ccc6b8fdda2079 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Tue, 1 May 2018 22:27:18 -0400 Subject: [PATCH 3/6] alternative approach to #1399 --- store.js | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/store.js b/store.js index abc2f1886f..7d43127251 100644 --- a/store.js +++ b/store.js @@ -49,31 +49,30 @@ assign(Store.prototype, { _sortComputedProperties: function() { var computed = this._computed; var sorted = this._sortedComputedProperties = []; - var cycles; var visited = blankObject(); + var lookup = blankObject(); - function visit(key, rootKey) { + function visit(key) { if (visited[key]) return; - - if (cycles[key]) { - throw new Error('Cyclical dependency detected. key: ' + key + ' rootKey: ' + rootKey); - } + visited[key] = true; var c = computed[key]; if (c) { - cycles[key] = true; - c.deps.forEach(function(dep) { - visit(dep, rootKey) + if (!lookup[key]) lookup[key] = blankObject(); + c.deps.forEach(dep => { + if (lookup[dep] && lookup[dep][key]) { + throw new Error(`Cyclical dependency detected between ${dep} <-> ${key}`); + } + lookup[key][dep] = true; + visit(dep); }); sorted.push(c); - visited[key] = true; } } for (var key in this._computed) { - cycles = blankObject(); - visit(key, key); + visit(key); } }, From 923c64d086849c96020ffa8101dcde96d5758709 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Tue, 1 May 2018 22:28:06 -0400 Subject: [PATCH 4/6] put test alongside other store tests --- .../store-computed-compute-graph/_config.js | 22 ------------------- .../store-computed-compute-graph/main.html | 0 test/store/index.js | 18 +++++++++++++++ 3 files changed, 18 insertions(+), 22 deletions(-) delete mode 100644 test/runtime/samples/store-computed-compute-graph/_config.js delete mode 100644 test/runtime/samples/store-computed-compute-graph/main.html diff --git a/test/runtime/samples/store-computed-compute-graph/_config.js b/test/runtime/samples/store-computed-compute-graph/_config.js deleted file mode 100644 index ba7ac02f8d..0000000000 --- a/test/runtime/samples/store-computed-compute-graph/_config.js +++ /dev/null @@ -1,22 +0,0 @@ -import { Store } from '../../../../store.js'; - -const store = new Store(); - -export default { - store, - - test(assert, component, target) { - store.compute('dep4', ['dep1', 'dep2', 'dep3'], (...args) => ['dep4'].concat(...args)); - store.compute('dep1', ['source'], (...args) => ['dep1'].concat(...args)); - store.compute('dep2', ['dep1'], (...args) => ['dep2'].concat(...args)); - store.compute('dep3', ['dep1', 'dep2'], (...args) => ['dep3'].concat(...args)); - store.set({source: 'source'}); - assert.equal(JSON.stringify(store.get().dep4), JSON.stringify([ - 'dep4', - 'dep1', 'source', - 'dep2', 'dep1', 'source', - 'dep3', 'dep1', 'source', - 'dep2', 'dep1', 'source' - ])); - } -}; diff --git a/test/runtime/samples/store-computed-compute-graph/main.html b/test/runtime/samples/store-computed-compute-graph/main.html deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/test/store/index.js b/test/store/index.js index 8bddb046f1..daba075bfb 100644 --- a/test/store/index.js +++ b/test/store/index.js @@ -144,6 +144,24 @@ describe('store', () => { store.compute('b', ['a'], a => a + 1); }, /Cyclical dependency detected/); }); + + it('does not falsely report cycles', () => { + const store = new Store(); + + store.compute('dep4', ['dep1', 'dep2', 'dep3'], (...args) => ['dep4'].concat(...args)); + store.compute('dep1', ['source'], (...args) => ['dep1'].concat(...args)); + store.compute('dep2', ['dep1'], (...args) => ['dep2'].concat(...args)); + store.compute('dep3', ['dep1', 'dep2'], (...args) => ['dep3'].concat(...args)); + store.set({source: 'source'}); + + assert.deepEqual(store.get().dep4, [ + 'dep4', + 'dep1', 'source', + 'dep2', 'dep1', 'source', + 'dep3', 'dep1', 'source', + 'dep2', 'dep1', 'source' + ]); + }); }); describe('immutable', () => { From 968b7956a48b8f2767e8fb39e9945d865bdd95f0 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Tue, 1 May 2018 22:31:43 -0400 Subject: [PATCH 5/6] on second thoughts, cycles is a better name still --- store.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/store.js b/store.js index 7d43127251..ddb7941527 100644 --- a/store.js +++ b/store.js @@ -50,7 +50,7 @@ assign(Store.prototype, { var computed = this._computed; var sorted = this._sortedComputedProperties = []; var visited = blankObject(); - var lookup = blankObject(); + var cycles = blankObject(); function visit(key) { if (visited[key]) return; @@ -59,12 +59,12 @@ assign(Store.prototype, { var c = computed[key]; if (c) { - if (!lookup[key]) lookup[key] = blankObject(); + if (!cycles[key]) cycles[key] = blankObject(); c.deps.forEach(dep => { - if (lookup[dep] && lookup[dep][key]) { + if (cycles[dep] && cycles[dep][key]) { throw new Error(`Cyclical dependency detected between ${dep} <-> ${key}`); } - lookup[key][dep] = true; + cycles[key][dep] = true; visit(dep); }); sorted.push(c); From 367f062e4e745b811f264c10caa16e9d8a35709a Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Wed, 2 May 2018 20:57:46 -0400 Subject: [PATCH 6/6] fix and simplify cycle detection --- store.js | 18 +++++++++--------- test/store/index.js | 5 +++-- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/store.js b/store.js index ddb7941527..c5b569c6a8 100644 --- a/store.js +++ b/store.js @@ -50,29 +50,29 @@ assign(Store.prototype, { var computed = this._computed; var sorted = this._sortedComputedProperties = []; var visited = blankObject(); - var cycles = blankObject(); + var currentKey; function visit(key) { - if (visited[key]) return; - visited[key] = true; - var c = computed[key]; if (c) { - if (!cycles[key]) cycles[key] = blankObject(); c.deps.forEach(dep => { - if (cycles[dep] && cycles[dep][key]) { + if (dep === currentKey) { throw new Error(`Cyclical dependency detected between ${dep} <-> ${key}`); } - cycles[key][dep] = true; + visit(dep); }); - sorted.push(c); + + if (!visited[key]) { + visited[key] = true; + sorted.push(c); + } } } for (var key in this._computed) { - visit(key); + visit(currentKey = key); } }, diff --git a/test/store/index.js b/test/store/index.js index daba075bfb..8d5dd5749c 100644 --- a/test/store/index.js +++ b/test/store/index.js @@ -141,8 +141,9 @@ describe('store', () => { assert.throws(() => { store.compute('a', ['b'], b => b + 1); - store.compute('b', ['a'], a => a + 1); - }, /Cyclical dependency detected/); + store.compute('b', ['c'], c => c + 1); + store.compute('c', ['a'], a => a + 1); + }, /Cyclical dependency detected between a <-> c/); }); it('does not falsely report cycles', () => {