From 7ea5b0f0c6cc32144e9843260fb7c952840a0fd4 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Mon, 30 Apr 2018 20:25:48 -0400 Subject: [PATCH 01/13] only overwrite this in event handlers for custom events - fixes #1390 --- src/compile/nodes/EventHandler.ts | 20 +++++++------ .../event-handler-each-this/_config.js | 28 +++++++++++++++++++ .../samples/event-handler-each-this/main.html | 3 ++ 3 files changed, 42 insertions(+), 9 deletions(-) create mode 100644 test/runtime/samples/event-handler-each-this/_config.js create mode 100644 test/runtime/samples/event-handler-each-this/main.html diff --git a/src/compile/nodes/EventHandler.ts b/src/compile/nodes/EventHandler.ts index 06d63f7f97..54dbd66157 100644 --- a/src/compile/nodes/EventHandler.ts +++ b/src/compile/nodes/EventHandler.ts @@ -77,16 +77,18 @@ export default class EventHandler extends Node { } } - this.args.forEach(arg => { - arg.overwriteThis(this.parent.var); - }); - - if (this.isCustomEvent && this.callee && this.callee.name === 'this') { - const node = this.callee.nodes[0]; - compiler.code.overwrite(node.start, node.end, this.parent.var, { - storeName: true, - contentOnly: true + if (this.isCustomEvent) { + this.args.forEach(arg => { + arg.overwriteThis(this.parent.var); }); + + if (this.callee && this.callee.name === 'this') { + const node = this.callee.nodes[0]; + compiler.code.overwrite(node.start, node.end, this.parent.var, { + storeName: true, + contentOnly: true + }); + } } } } \ No newline at end of file diff --git a/test/runtime/samples/event-handler-each-this/_config.js b/test/runtime/samples/event-handler-each-this/_config.js new file mode 100644 index 0000000000..e6d1056e7e --- /dev/null +++ b/test/runtime/samples/event-handler-each-this/_config.js @@ -0,0 +1,28 @@ +export default { + data: { + items: ['foo', 'bar', 'baz'], + }, + + html: ` + + + + `, + + test(assert, component, target, window) { + const buttons = target.querySelectorAll('button'); + const event = new window.MouseEvent('click'); + + const clicked = []; + + component.on('clicked', event => { + clicked.push(event.node); + }); + + buttons[1].dispatchEvent(event); + + assert.equal(clicked.length, 1); + assert.equal(clicked[0].nodeName, 'BUTTON'); + assert.equal(clicked[0].textContent, 'bar'); + } +}; diff --git a/test/runtime/samples/event-handler-each-this/main.html b/test/runtime/samples/event-handler-each-this/main.html new file mode 100644 index 0000000000..9e5ea88a50 --- /dev/null +++ b/test/runtime/samples/event-handler-each-this/main.html @@ -0,0 +1,3 @@ +{#each items as item} + +{/each} \ No newline at end of file From d8ca9676e4487f81ae61ad18a3b5f28d01141565 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Mon, 30 Apr 2018 21:02:30 -0400 Subject: [PATCH 02/13] allow destruction of components with bind:offsetWidth etc --- src/shared/dom.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/shared/dom.js b/src/shared/dom.js index 54778d1aaf..2490fe0be8 100644 --- a/src/shared/dom.js +++ b/src/shared/dom.js @@ -203,8 +203,11 @@ export function addResizeListener(element, fn) { object.setAttribute('style', 'display: block; position: absolute; top: 0; left: 0; height: 100%; width: 100%; overflow: hidden; pointer-events: none; z-index: -1;'); object.type = 'text/html'; + let win; + object.onload = () => { - object.contentDocument.defaultView.addEventListener('resize', fn); + win = object.contentDocument.defaultView; + win.addEventListener('resize', fn); }; if (/Trident/.test(navigator.userAgent)) { @@ -217,7 +220,7 @@ export function addResizeListener(element, fn) { return { cancel: () => { - object.contentDocument.defaultView.removeEventListener('resize', fn); + win.removeEventListener('resize', fn); element.removeChild(object); } }; From 4106095033eef33772f8859d3b19481e564fde52 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Mon, 30 Apr 2018 21:03:34 -0400 Subject: [PATCH 03/13] update test --- test/js/samples/bind-width-height/expected-bundle.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/test/js/samples/bind-width-height/expected-bundle.js b/test/js/samples/bind-width-height/expected-bundle.js index 4d003e7e85..0787252e8d 100644 --- a/test/js/samples/bind-width-height/expected-bundle.js +++ b/test/js/samples/bind-width-height/expected-bundle.js @@ -26,8 +26,11 @@ function addResizeListener(element, fn) { object.setAttribute('style', 'display: block; position: absolute; top: 0; left: 0; height: 100%; width: 100%; overflow: hidden; pointer-events: none; z-index: -1;'); object.type = 'text/html'; + let win; + object.onload = () => { - object.contentDocument.defaultView.addEventListener('resize', fn); + win = object.contentDocument.defaultView; + win.addEventListener('resize', fn); }; if (/Trident/.test(navigator.userAgent)) { @@ -40,7 +43,7 @@ function addResizeListener(element, fn) { return { cancel: () => { - object.contentDocument.defaultView.removeEventListener('resize', fn); + win.removeEventListener('resize', fn); element.removeChild(object); } }; From 886acc7993162a4992e1af0ca89073e92808c3d6 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Mon, 30 Apr 2018 21:37:56 -0400 Subject: [PATCH 04/13] -> v2.4.1 --- CHANGELOG.md | 4 ++++ package.json | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c15d0aaa2..002d5c7039 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Svelte changelog +## 2.4.1 + +* Fix DOM event context ([#1390](https://github.com/sveltejs/svelte/issues/1390)) + ## 2.4.0 * Integrate CLI ([#1360](https://github.com/sveltejs/svelte/issues/1360)) diff --git a/package.json b/package.json index 111f091eba..c2ede3bbad 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "svelte", - "version": "2.4.0", + "version": "2.4.1", "description": "The magical disappearing UI framework", "main": "compiler/svelte.js", "bin": { From cb1978e93725d10fb353e95ef453f58016bb1665 Mon Sep 17 00:00:00 2001 From: Brian Takita Date: Tue, 1 May 2018 21:20:05 -0400 Subject: [PATCH 05/13] 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 c7bd87bd836715f9e234e06d4d14cc4324b2ae00 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Tue, 1 May 2018 21:24:22 -0400 Subject: [PATCH 06/13] evaluate each block key in child scope - fixes #1397 --- src/compile/nodes/EachBlock.ts | 8 +++---- .../dev-warning-missing-data-each/_config.js | 22 +++++++++++++++++++ .../dev-warning-missing-data-each/main.html | 3 +++ 3 files changed, 29 insertions(+), 4 deletions(-) create mode 100644 test/runtime/samples/dev-warning-missing-data-each/_config.js create mode 100644 test/runtime/samples/dev-warning-missing-data-each/main.html diff --git a/src/compile/nodes/EachBlock.ts b/src/compile/nodes/EachBlock.ts index 6d771dbf87..35376c9e2b 100644 --- a/src/compile/nodes/EachBlock.ts +++ b/src/compile/nodes/EachBlock.ts @@ -31,10 +31,6 @@ export default class EachBlock extends Node { this.context = info.context.name || 'each'; // TODO this is used to facilitate binding; currently fails with destructuring this.index = info.index; - this.key = info.key - ? new Expression(compiler, this, scope, info.key) - : null; - this.scope = scope.child(); this.contexts = []; @@ -50,6 +46,10 @@ export default class EachBlock extends Node { this.scope.add(this.index, dependencies); } + this.key = info.key + ? new Expression(compiler, this, this.scope, info.key) + : null; + this.children = mapChildren(compiler, this, this.scope, info.children); this.else = info.else diff --git a/test/runtime/samples/dev-warning-missing-data-each/_config.js b/test/runtime/samples/dev-warning-missing-data-each/_config.js new file mode 100644 index 0000000000..a8eb59216a --- /dev/null +++ b/test/runtime/samples/dev-warning-missing-data-each/_config.js @@ -0,0 +1,22 @@ +export default { + dev: true, + + data: { + letters: [ + { + id: 1, + char: 'a', + }, + { + id: 2, + char: 'b', + }, + { + id: 3, + char: 'c', + }, + ], + }, + + warnings: [], +}; diff --git a/test/runtime/samples/dev-warning-missing-data-each/main.html b/test/runtime/samples/dev-warning-missing-data-each/main.html new file mode 100644 index 0000000000..6b6a72204e --- /dev/null +++ b/test/runtime/samples/dev-warning-missing-data-each/main.html @@ -0,0 +1,3 @@ +{#each letters as letter (letter.id)} +
{letter.char}
+{/each} \ No newline at end of file From 8f476c1a9c3ac3d7c97902d9f958fc184f382a38 Mon Sep 17 00:00:00 2001 From: Brian Takita Date: Tue, 1 May 2018 21:28:37 -0400 Subject: [PATCH 07/13] 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 53c596ca85d88088925dc8cd118d760d0b041c6d Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Tue, 1 May 2018 21:42:36 -0400 Subject: [PATCH 08/13] only update component props if they are dynamic - fixes #1394 --- src/compile/nodes/Attribute.ts | 12 +- src/compile/nodes/Component.ts | 6 - .../component-static-array/expected-bundle.js | 183 ++++++++++++++++++ .../component-static-array/expected.js | 60 ++++++ .../samples/component-static-array/input.html | 9 + 5 files changed, 255 insertions(+), 15 deletions(-) create mode 100644 test/js/samples/component-static-array/expected-bundle.js create mode 100644 test/js/samples/component-static-array/expected.js create mode 100644 test/js/samples/component-static-array/input.html diff --git a/src/compile/nodes/Attribute.ts b/src/compile/nodes/Attribute.ts index 37de5068a9..b933ca2e7c 100644 --- a/src/compile/nodes/Attribute.ts +++ b/src/compile/nodes/Attribute.ts @@ -66,11 +66,7 @@ export default class Attribute extends Node { return expression; }); - // TODO this would be better, but it breaks some stuff - // this.isDynamic = this.dependencies.size > 0; - this.isDynamic = this.chunks.length === 1 - ? this.chunks[0].type !== 'Text' - : this.chunks.length > 1; + this.isDynamic = this.dependencies.size > 0; this.shouldCache = this.isDynamic ? this.chunks.length === 1 @@ -82,7 +78,7 @@ export default class Attribute extends Node { getValue() { if (this.isTrue) return true; - if (this.chunks.length === 0) return `''`; + if (this.chunks.length === 0) return `""`; if (this.chunks.length === 1) { return this.chunks[0].type === 'Text' @@ -252,9 +248,7 @@ export default class Attribute extends Node { ); } } else { - const value = this.isTrue - ? 'true' - : this.chunks.length === 0 ? `""` : stringify(this.chunks[0].data); + const value = this.getValue(); const statement = ( isLegacyInputType diff --git a/src/compile/nodes/Component.ts b/src/compile/nodes/Component.ts index 1a872fe6ce..9996c19a92 100644 --- a/src/compile/nodes/Component.ts +++ b/src/compile/nodes/Component.ts @@ -209,12 +209,6 @@ export default class Component extends Node { .join(' || ')}) ${name_changes}.${attribute.name} = ${attribute.getValue()}; `); } - - else { - // TODO this is an odd situation to encounter – I *think* it should only happen with - // each block indices, in which case it may be possible to optimise this - updates.push(`${name_changes}.${attribute.name} = ${attribute.getValue()};`); - } }); } } diff --git a/test/js/samples/component-static-array/expected-bundle.js b/test/js/samples/component-static-array/expected-bundle.js new file mode 100644 index 0000000000..a210f5ce42 --- /dev/null +++ b/test/js/samples/component-static-array/expected-bundle.js @@ -0,0 +1,183 @@ +function noop() {} + +function assign(tar, src) { + for (var k in src) tar[k] = src[k]; + return tar; +} + +function blankObject() { + return Object.create(null); +} + +function destroy(detach) { + this.destroy = noop; + this.fire('destroy'); + this.set = noop; + + if (detach !== false) this._fragment.u(); + this._fragment.d(); + this._fragment = null; + this._state = {}; +} + +function _differs(a, b) { + return a != a ? b == b : a !== b || ((a && typeof a === 'object') || typeof a === 'function'); +} + +function fire(eventName, data) { + var handlers = + eventName in this._handlers && this._handlers[eventName].slice(); + if (!handlers) return; + + for (var i = 0; i < handlers.length; i += 1) { + var handler = handlers[i]; + + if (!handler.__calling) { + handler.__calling = true; + handler.call(this, data); + handler.__calling = false; + } + } +} + +function get() { + return this._state; +} + +function init(component, options) { + component._handlers = blankObject(); + component._bind = options._bind; + + component.options = options; + component.root = options.root || component; + component.store = component.root.store || options.store; +} + +function on(eventName, handler) { + var handlers = this._handlers[eventName] || (this._handlers[eventName] = []); + handlers.push(handler); + + return { + cancel: function() { + var index = handlers.indexOf(handler); + if (~index) handlers.splice(index, 1); + } + }; +} + +function set(newState) { + this._set(assign({}, newState)); + if (this.root._lock) return; + this.root._lock = true; + callAll(this.root._beforecreate); + callAll(this.root._oncreate); + callAll(this.root._aftercreate); + this.root._lock = false; +} + +function _set(newState) { + var oldState = this._state, + changed = {}, + dirty = false; + + for (var key in newState) { + if (this._differs(newState[key], oldState[key])) changed[key] = dirty = true; + } + if (!dirty) return; + + this._state = assign(assign({}, oldState), newState); + this._recompute(changed, this._state); + if (this._bind) this._bind(changed, this._state); + + if (this._fragment) { + this.fire("state", { changed: changed, current: this._state, previous: oldState }); + this._fragment.p(changed, this._state); + this.fire("update", { changed: changed, current: this._state, previous: oldState }); + } +} + +function callAll(fns) { + while (fns && fns.length) fns.shift()(); +} + +function _mount(target, anchor) { + this._fragment[this._fragment.i ? 'i' : 'm'](target, anchor || null); +} + +function _unmount() { + if (this._fragment) this._fragment.u(); +} + +var proto = { + destroy, + get, + fire, + on, + set, + _recompute: noop, + _set, + _mount, + _unmount, + _differs +}; + +/* generated by Svelte vX.Y.Z */ + +var Nested = window.Nested; + +function create_main_fragment(component, ctx) { + + var nested_initial_data = { foo: [1, 2, 3] }; + var nested = new Nested({ + root: component.root, + data: nested_initial_data + }); + + return { + c() { + nested._fragment.c(); + }, + + m(target, anchor) { + nested._mount(target, anchor); + }, + + p: noop, + + u() { + nested._unmount(); + }, + + d() { + nested.destroy(false); + } + }; +} + +function SvelteComponent(options) { + init(this, options); + this._state = assign({}, options.data); + + if (!options.root) { + this._oncreate = []; + this._beforecreate = []; + this._aftercreate = []; + } + + this._fragment = create_main_fragment(this, this._state); + + if (options.target) { + this._fragment.c(); + this._mount(options.target, options.anchor); + + this._lock = true; + callAll(this._beforecreate); + callAll(this._oncreate); + callAll(this._aftercreate); + this._lock = false; + } +} + +assign(SvelteComponent.prototype, proto); + +export default SvelteComponent; diff --git a/test/js/samples/component-static-array/expected.js b/test/js/samples/component-static-array/expected.js new file mode 100644 index 0000000000..cba2d4d947 --- /dev/null +++ b/test/js/samples/component-static-array/expected.js @@ -0,0 +1,60 @@ +/* generated by Svelte vX.Y.Z */ +import { assign, callAll, init, noop, proto } from "svelte/shared.js"; + +var Nested = window.Nested; + +function create_main_fragment(component, ctx) { + + var nested_initial_data = { foo: [1, 2, 3] }; + var nested = new Nested({ + root: component.root, + data: nested_initial_data + }); + + return { + c() { + nested._fragment.c(); + }, + + m(target, anchor) { + nested._mount(target, anchor); + }, + + p: noop, + + u() { + nested._unmount(); + }, + + d() { + nested.destroy(false); + } + }; +} + +function SvelteComponent(options) { + init(this, options); + this._state = assign({}, options.data); + + if (!options.root) { + this._oncreate = []; + this._beforecreate = []; + this._aftercreate = []; + } + + this._fragment = create_main_fragment(this, this._state); + + if (options.target) { + this._fragment.c(); + this._mount(options.target, options.anchor); + + this._lock = true; + callAll(this._beforecreate); + callAll(this._oncreate); + callAll(this._aftercreate); + this._lock = false; + } +} + +assign(SvelteComponent.prototype, proto); +export default SvelteComponent; \ No newline at end of file diff --git a/test/js/samples/component-static-array/input.html b/test/js/samples/component-static-array/input.html new file mode 100644 index 0000000000..f87ea0a7e5 --- /dev/null +++ b/test/js/samples/component-static-array/input.html @@ -0,0 +1,9 @@ + + + \ No newline at end of file From cacec0c63ab7826321c0d4ab42fc76d564b43db3 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Tue, 1 May 2018 21:55:08 -0400 Subject: [PATCH 09/13] create key before testing for the existence of key. doh --- src/compile/nodes/EachBlock.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/compile/nodes/EachBlock.ts b/src/compile/nodes/EachBlock.ts index 35376c9e2b..d9c93fc036 100644 --- a/src/compile/nodes/EachBlock.ts +++ b/src/compile/nodes/EachBlock.ts @@ -40,16 +40,16 @@ export default class EachBlock extends Node { this.scope.add(context.key.name, this.expression.dependencies); }); + this.key = info.key + ? new Expression(compiler, this, this.scope, info.key) + : null; + if (this.index) { // index can only change if this is a keyed each block const dependencies = this.key ? this.expression.dependencies : []; this.scope.add(this.index, dependencies); } - this.key = info.key - ? new Expression(compiler, this, this.scope, info.key) - : null; - this.children = mapChildren(compiler, this, this.scope, info.children); this.else = info.else From 71260f9918d33f044bb5d41784be634a511e0f01 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Tue, 1 May 2018 22:27:18 -0400 Subject: [PATCH 10/13] 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 c176506086099803594ae316780ac31f9e39c325 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Tue, 1 May 2018 22:28:06 -0400 Subject: [PATCH 11/13] 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 6742c8c5e9a28558257b1a230f4cf8be7b66186c Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Tue, 1 May 2018 22:31:43 -0400 Subject: [PATCH 12/13] 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 fd7063e84a14fc16ad73444a6b0fdd0adc332d0d Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Wed, 2 May 2018 20:57:46 -0400 Subject: [PATCH 13/13] 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', () => {