From cecdcdddefddfcce3124e0b4b0f983aa1ab22e27 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Wed, 13 May 2015 13:03:55 -0400 Subject: [PATCH 1/2] failing tests for rerender bug --- .../integration/component_invocation_test.js | 100 ++++++++++++++++++ 1 file changed, 100 insertions(+) diff --git a/packages/ember-htmlbars/tests/integration/component_invocation_test.js b/packages/ember-htmlbars/tests/integration/component_invocation_test.js index 76b87745a3d..f146434bb3d 100644 --- a/packages/ember-htmlbars/tests/integration/component_invocation_test.js +++ b/packages/ember-htmlbars/tests/integration/component_invocation_test.js @@ -451,3 +451,103 @@ QUnit.test('implementing `render` allows pushing into a string buffer', function equal(view.$('#zomg').text(), 'Whoop!'); }); + +QUnit.test("comopnent should rerender when a property is changed during children's rendering", function() { + var outer, middle; + + registry.register('component:x-outer', Component.extend({ + value: 1, + grabReference: Ember.on('init', function() { + outer = this; + }) + })); + + registry.register('component:x-middle', Component.extend({ + grabReference: Ember.on('init', function() { + middle = this; + }) + })); + + registry.register('component:x-inner', Component.extend({ + pushDataUp: Ember.observer('value', function() { + middle.set('value', this.get('value')); + }) + })); + + registry.register('template:components/x-outer', compile('{{#x-middle}}{{x-inner value=value}}{{/x-middle}}')); + registry.register('template:components/x-middle', compile('
{{value}}
{{yield}}')); + registry.register('template:components/x-inner', compile('
{{value}}
')); + + + view = EmberView.extend({ + template: compile('{{x-outer}}'), + container: container + }).create(); + + runAppend(view); + + equal(view.$('#inner-value').text(), '1', 'initial render of inner'); + equal(view.$('#middle-value').text(), '1', 'initial render of middle'); + + run(() => outer.set('value', 2)); + + equal(view.$('#inner-value').text(), '2', 'second render of inner'); + equal(view.$('#middle-value').text(), '2', 'second render of middle'); + + run(() => outer.set('value', 3)); + + equal(view.$('#inner-value').text(), '3', 'third render of inner'); + equal(view.$('#middle-value').text(), '3', 'third render of middle'); + +}); + +QUnit.test("comopnent should rerender when a property (with a default) is changed during children's rendering", function() { + var outer, middle; + + registry.register('component:x-outer', Component.extend({ + value: 1, + grabReference: Ember.on('init', function() { + outer = this; + }) + })); + + registry.register('component:x-middle', Component.extend({ + value: null, + grabReference: Ember.on('init', function() { + middle = this; + }) + })); + + registry.register('component:x-inner', Component.extend({ + value: null, + pushDataUp: Ember.observer('value', function() { + middle.set('value', this.get('value')); + }) + })); + + registry.register('template:components/x-outer', compile('{{#x-middle}}{{x-inner value=value}}{{/x-middle}}')); + registry.register('template:components/x-middle', compile('
{{value}}
{{yield}}')); + registry.register('template:components/x-inner', compile('
{{value}}
')); + + + view = EmberView.extend({ + template: compile('{{x-outer}}'), + container: container + }).create(); + + runAppend(view); + + equal(view.$('#inner-value').text(), '1', 'initial render of inner'); + equal(view.$('#middle-value').text(), '1', 'initial render of middle'); + + run(() => outer.set('value', 2)); + + equal(view.$('#inner-value').text(), '2', 'second render of inner'); + equal(view.$('#middle-value').text(), '2', 'second render of middle'); + + run(() => outer.set('value', 3)); + + equal(view.$('#inner-value').text(), '3', 'third render of inner'); + equal(view.$('#middle-value').text(), '3', 'third render of middle'); + +}); From 467eb6d75029bfeb9264ce9db7322b5c691b3bd7 Mon Sep 17 00:00:00 2001 From: Tom Dale Date: Wed, 13 May 2015 13:35:32 -0700 Subject: [PATCH 2/2] Support rerendering parent render nodes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In idiomatic Ember, you should render “data-down”: that is, parent components should modify their children, but child components should not mutate their parents, particularly during the rendering process. By violating this rule, it is easy to trigger infinite loops or multiple rendering passes that dramatically harm performance. In certain situations, it was possible in previous versions of Ember to rely on this behavior, and several regressions were reported. This commit adds support for the old behavior and now emits a deprecation to help users migrate to a more performant architecture. In particular, we now track which render nodes have been rendered during a rendering pass. If we detect that a node has been dirtied after it has been rendered (and thus requires an additional rendering pass), we now schedule a re-render appropriately but emit a warning. Note that there *are* legitimate use cases for multi-pass rendering (e.g., measuring rendered text to layout a parent container component) and we intend to add a public API for handling this use case that can still prevent inadvertent pathological performance. --- package.json | 2 +- packages/ember-htmlbars/lib/env.js | 2 ++ packages/ember-htmlbars/lib/hooks/did-render-node.js | 3 +++ .../ember-htmlbars/lib/hooks/link-render-node.js | 4 ++-- packages/ember-htmlbars/lib/morphs/morph.js | 6 ++++++ packages/ember-htmlbars/lib/system/render-view.js | 1 + packages/ember-htmlbars/lib/utils/subscribe.js | 6 +++--- .../tests/integration/component_invocation_test.js | 6 +++++- packages/ember-metal-views/lib/renderer.js | 1 + packages/ember-routing-views/lib/views/outlet.js | 2 +- packages/ember-views/lib/views/states/has_element.js | 3 ++- packages/ember-views/lib/views/view.js | 12 +++++++++++- 12 files changed, 38 insertions(+), 10 deletions(-) create mode 100644 packages/ember-htmlbars/lib/hooks/did-render-node.js diff --git a/package.json b/package.json index c159967f507..655efdbec5d 100644 --- a/package.json +++ b/package.json @@ -23,7 +23,7 @@ "express": "^4.5.0", "github": "^0.2.3", "glob": "~4.3.2", - "htmlbars": "0.13.18", + "htmlbars": "0.13.19", "qunit-extras": "^1.3.0", "qunitjs": "^1.16.0", "route-recognizer": "0.1.5", diff --git a/packages/ember-htmlbars/lib/env.js b/packages/ember-htmlbars/lib/env.js index 4da6565f368..81da889dab4 100644 --- a/packages/ember-htmlbars/lib/env.js +++ b/packages/ember-htmlbars/lib/env.js @@ -18,6 +18,7 @@ import getValue from "ember-htmlbars/hooks/get-value"; import getCellOrValue from "ember-htmlbars/hooks/get-cell-or-value"; import cleanupRenderNode from "ember-htmlbars/hooks/cleanup-render-node"; import destroyRenderNode from "ember-htmlbars/hooks/destroy-render-node"; +import didRenderNode from "ember-htmlbars/hooks/did-render-node"; import willCleanupTree from "ember-htmlbars/hooks/will-cleanup-tree"; import didCleanupTree from "ember-htmlbars/hooks/did-cleanup-tree"; import classify from "ember-htmlbars/hooks/classify"; @@ -53,6 +54,7 @@ merge(emberHooks, { destroyRenderNode: destroyRenderNode, willCleanupTree: willCleanupTree, didCleanupTree: didCleanupTree, + didRenderNode: didRenderNode, classify: classify, component: component, lookupHelper: lookupHelper, diff --git a/packages/ember-htmlbars/lib/hooks/did-render-node.js b/packages/ember-htmlbars/lib/hooks/did-render-node.js new file mode 100644 index 00000000000..524e212c521 --- /dev/null +++ b/packages/ember-htmlbars/lib/hooks/did-render-node.js @@ -0,0 +1,3 @@ +export default function didRenderNode(morph, env) { + env.renderedNodes[morph.guid] = true; +} diff --git a/packages/ember-htmlbars/lib/hooks/link-render-node.js b/packages/ember-htmlbars/lib/hooks/link-render-node.js index a407495b46b..f5f9bee9469 100644 --- a/packages/ember-htmlbars/lib/hooks/link-render-node.js +++ b/packages/ember-htmlbars/lib/hooks/link-render-node.js @@ -34,13 +34,13 @@ export default function linkRenderNode(renderNode, env, scope, path, params, has if (params && params.length) { for (var i = 0; i < params.length; i++) { - subscribe(renderNode, scope, params[i]); + subscribe(renderNode, env, scope, params[i]); } } if (hash) { for (var key in hash) { - subscribe(renderNode, scope, hash[key]); + subscribe(renderNode, env, scope, hash[key]); } } diff --git a/packages/ember-htmlbars/lib/morphs/morph.js b/packages/ember-htmlbars/lib/morphs/morph.js index 0b8f18a8c86..459773f2889 100644 --- a/packages/ember-htmlbars/lib/morphs/morph.js +++ b/packages/ember-htmlbars/lib/morphs/morph.js @@ -2,6 +2,7 @@ import DOMHelper from "dom-helper"; import o_create from 'ember-metal/platform/create'; var HTMLBarsMorph = DOMHelper.prototype.MorphClass; +let guid = 1; function EmberMorph(DOMHelper, contextualElement) { this.HTMLBarsMorph$constructor(DOMHelper, contextualElement); @@ -9,6 +10,7 @@ function EmberMorph(DOMHelper, contextualElement) { this.emberView = null; this.emberToDestroy = null; this.streamUnsubscribers = null; + this.guid = guid++; // A component can become dirty either because one of its // attributes changed, or because it was re-rendered. If any part @@ -49,4 +51,8 @@ proto.cleanup = function() { } }; +proto.didRender = function(env, scope) { + env.renderedNodes[this.guid] = true; +}; + export default EmberMorph; diff --git a/packages/ember-htmlbars/lib/system/render-view.js b/packages/ember-htmlbars/lib/system/render-view.js index fa1fb9a5cee..365fd201bf0 100644 --- a/packages/ember-htmlbars/lib/system/render-view.js +++ b/packages/ember-htmlbars/lib/system/render-view.js @@ -7,6 +7,7 @@ export function renderHTMLBarsBlock(view, block, renderNode) { var env = { lifecycleHooks: [], renderedViews: [], + renderedNodes: {}, view: view, outletState: view.outletState, container: view.container, diff --git a/packages/ember-htmlbars/lib/utils/subscribe.js b/packages/ember-htmlbars/lib/utils/subscribe.js index ea5b7171359..08bf80bde10 100644 --- a/packages/ember-htmlbars/lib/utils/subscribe.js +++ b/packages/ember-htmlbars/lib/utils/subscribe.js @@ -1,6 +1,6 @@ -import { isStream } from "ember-metal/streams/utils"; +import { isStream, labelFor } from "ember-metal/streams/utils"; -export default function subscribe(node, scope, stream) { +export default function subscribe(node, env, scope, stream) { if (!isStream(stream)) { return; } var component = scope.component; var unsubscribers = node.streamUnsubscribers = node.streamUnsubscribers || []; @@ -21,6 +21,6 @@ export default function subscribe(node, scope, stream) { node.shouldReceiveAttrs = true; } - node.ownerNode.emberView.scheduleRevalidate(); + node.ownerNode.emberView.scheduleRevalidate(node, labelFor(stream)); })); } diff --git a/packages/ember-htmlbars/tests/integration/component_invocation_test.js b/packages/ember-htmlbars/tests/integration/component_invocation_test.js index f146434bb3d..97229141942 100644 --- a/packages/ember-htmlbars/tests/integration/component_invocation_test.js +++ b/packages/ember-htmlbars/tests/integration/component_invocation_test.js @@ -453,6 +453,8 @@ QUnit.test('implementing `render` allows pushing into a string buffer', function }); QUnit.test("comopnent should rerender when a property is changed during children's rendering", function() { + expectDeprecation(/twice in a single render/); + var outer, middle; registry.register('component:x-outer', Component.extend({ @@ -502,6 +504,8 @@ QUnit.test("comopnent should rerender when a property is changed during children }); QUnit.test("comopnent should rerender when a property (with a default) is changed during children's rendering", function() { + expectDeprecation(/modified value twice in a single render/); + var outer, middle; registry.register('component:x-outer', Component.extend({ @@ -538,7 +542,7 @@ QUnit.test("comopnent should rerender when a property (with a default) is change runAppend(view); equal(view.$('#inner-value').text(), '1', 'initial render of inner'); - equal(view.$('#middle-value').text(), '1', 'initial render of middle'); + equal(view.$('#middle-value').text(), '', 'initial render of middle (observers do not run during init)'); run(() => outer.set('value', 2)); diff --git a/packages/ember-metal-views/lib/renderer.js b/packages/ember-metal-views/lib/renderer.js index 70da9ab8be7..6b36bc63d89 100755 --- a/packages/ember-metal-views/lib/renderer.js +++ b/packages/ember-metal-views/lib/renderer.js @@ -89,6 +89,7 @@ Renderer.prototype.ensureViewNotRendering = Renderer.prototype.clearRenderedViews = function Renderer_clearRenderedViews(env) { + env.renderedNodes = {}; env.renderedViews.length = 0; }; diff --git a/packages/ember-routing-views/lib/views/outlet.js b/packages/ember-routing-views/lib/views/outlet.js index 5eb53da8231..9f65056b16e 100644 --- a/packages/ember-routing-views/lib/views/outlet.js +++ b/packages/ember-routing-views/lib/views/outlet.js @@ -26,7 +26,7 @@ export var CoreOutletView = View.extend({ this.dirtyOutlets(); this._outlets = []; - this.scheduleRevalidate(); + this.scheduleRevalidate(null, null); } }, diff --git a/packages/ember-views/lib/views/states/has_element.js b/packages/ember-views/lib/views/states/has_element.js index e8284cd5681..56bbaf2be13 100644 --- a/packages/ember-views/lib/views/states/has_element.js +++ b/packages/ember-views/lib/views/states/has_element.js @@ -40,7 +40,8 @@ merge(hasElement, { } node.isDirty = true; }); - renderNode.ownerNode.emberView.scheduleRevalidate(); + + renderNode.ownerNode.emberView.scheduleRevalidate(renderNode, view.toString(), "rerendering"); }, cleanup(view) { diff --git a/packages/ember-views/lib/views/view.js b/packages/ember-views/lib/views/view.js index 1c1341b0d4b..13af4f69806 100644 --- a/packages/ember-views/lib/views/view.js +++ b/packages/ember-views/lib/views/view.js @@ -1330,7 +1330,17 @@ var View = CoreView.extend( this.scheduledRevalidation = false; }, - scheduleRevalidate() { + scheduleRevalidate(node, label, manualRerender) { + if (node && !this._dispatching && node.guid in this.env.renderedNodes) { + if (manualRerender) { + Ember.deprecate(`You manually rerendered ${label} (a parent component) from a child component during the rendering process. This rarely worked in Ember 1.x and will be removed in Ember 2.0`); + } else { + Ember.deprecate(`You modified ${label} twice in a single render. This was unreliable in Ember 1.x and will be removed in Ember 2.0`); + } + run.scheduleOnce('render', this, this.revalidate); + return; + } + Ember.deprecate(`A property of ${this} was modified inside the ${this._dispatching} hook. You should never change properties on components, services or models during ${this._dispatching} because it causes significant performance degradation.`, !this._dispatching); if (!this.scheduledRevalidation || this._dispatching) {