Skip to content

Commit

Permalink
[BUGFIX beta] Only call attr hooks in FastBoot
Browse files Browse the repository at this point in the history 
This is a follow-up to emberjs#14233.

At the September 2016 core team face-to-face meeting, we discussed and
clarified the intended features for life-cycle hooks in non-iterative
environments (i.e. FastBoot).

We agreed that components on FastBoot should only react to data changes
via the `didReceiveAttrs` and `didUpdateAttrs` hooks. (There is also
deprecated `didInitAttrs` hook.)

The main change is that we have been incorrectly calling `willRender`
and `willUpdate` in non-interactive mode so far (at least since we last
checked and added the tests). The problem is that `{will,did}Render` and
`{will,did}Update` are intended to work as a pair so running one but not
the other could introduce unexpected bugs for applications.
  • Loading branch information
@chancancode @mixonic
chancancode authored and mixonic committed Oct 22, 2016
1 parent 8f03119 commit 4c42b0d
Show file tree
Hide file tree
Showing 3 changed files with 56 additions and 122 deletions.
16 changes: 9 additions & 7 deletions packages/ember-glimmer/lib/syntax/curly-component.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,9 @@ class CurlyComponentManager {
component.trigger('didInitAttrs', { attrs });
component.trigger('didReceiveAttrs', { newAttrs: attrs });

component.trigger('willRender');
if (environment.isInteractive) {
component.trigger('willRender');
}

let bucket = new ComponentStateBucket(environment, component, processedArgs, finalizer);

Expand Down Expand Up @@ -332,7 +334,7 @@ class CurlyComponentManager {
}

update(bucket, _, dynamicScope) {
let { component, args, argsRevision } = bucket;
let { component, args, argsRevision, environment } = bucket;

bucket.finalizer = _instrumentStart('render.component', rerenderInstrumentDetails, component);

Expand All @@ -352,8 +354,10 @@ class CurlyComponentManager {
component.trigger('didReceiveAttrs', { oldAttrs, newAttrs });
}

component.trigger('willUpdate');
component.trigger('willRender');
if (environment.isInteractive) {
component.trigger('willUpdate');
component.trigger('willRender');
}
}

didUpdateLayout(bucket) {
Expand Down Expand Up @@ -386,11 +390,9 @@ class TopComponentManager extends CurlyComponentManager {
component.trigger('didReceiveAttrs');

if (environment.isInteractive) {
component.trigger('willInsertElement');
component.trigger('willRender');
}

component.trigger('willRender');

processComponentInitializationAssertions(component, {});

return new ComponentStateBucket(environment, component, args, finalizer);
Expand Down
147 changes: 38 additions & 109 deletions packages/ember-glimmer/tests/integration/components/life-cycle-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,8 @@ class LifeCycleHooksTest extends RenderingTest {
this.assert.equal(instance._state, expectedState, `within ${hookName} the expected _state is ${expectedState}`);
};

let { isInteractive } = this;

let ComponentClass = this.ComponentClass.extend({
init() {
expectDeprecation(() => { this._super(...arguments); },
Expand Down Expand Up @@ -153,7 +155,12 @@ class LifeCycleHooksTest extends RenderingTest {
assertState('didReceiveAttrs', 'preRender', this);
} else {
assertElement('didReceiveAttrs', this);
assertState('didReceiveAttrs', 'inDOM', this);

if (isInteractive) {
assertState('didReceiveAttrs', 'inDOM', this);
} else {
assertState('didReceiveAttrs', 'hasElement', this);
}
}
},

Expand Down Expand Up @@ -194,8 +201,12 @@ class LifeCycleHooksTest extends RenderingTest {
didUpdateAttrs(options) {
pushHook('didUpdateAttrs', options);
assertParentView('didUpdateAttrs', this);
assertElement('didUpdateAttrs', this);
assertState('didUpdateAttrs', 'inDOM', this);

if (isInteractive) {
assertState('didUpdateAttrs', 'inDOM', this);
} else {
assertState('didUpdateAttrs', 'hasElement', this);
}
},

willUpdate(options) {
Expand Down Expand Up @@ -323,17 +334,14 @@ class LifeCycleHooksTest extends RenderingTest {
['the-top', 'init'],
['the-top', 'didInitAttrs', { attrs: topAttrs }],
['the-top', 'didReceiveAttrs', { newAttrs: topAttrs }],
['the-top', 'willRender'],

['the-middle', 'init'],
['the-middle', 'didInitAttrs', { attrs: middleAttrs }],
['the-middle', 'didReceiveAttrs', { newAttrs: middleAttrs }],
['the-middle', 'willRender'],

['the-bottom', 'init'],
['the-bottom', 'didInitAttrs', { attrs: bottomAttrs }],
['the-bottom', 'didReceiveAttrs', { newAttrs: bottomAttrs }],
['the-bottom', 'willRender']
['the-bottom', 'didReceiveAttrs', { newAttrs: bottomAttrs }]
]
});

Expand Down Expand Up @@ -367,16 +375,7 @@ class LifeCycleHooksTest extends RenderingTest {
['the-top', 'didRender']
],

nonInteractive: [
['the-top', 'willUpdate'],
['the-top', 'willRender'],

['the-middle', 'willUpdate'],
['the-middle', 'willRender'],

['the-bottom', 'willUpdate'],
['the-bottom', 'willRender']
]
nonInteractive: []
});

this.runTask(() => this.components['the-middle'].rerender());
Expand Down Expand Up @@ -404,14 +403,7 @@ class LifeCycleHooksTest extends RenderingTest {
['the-top', 'didRender']
],

nonInteractive: [
// Sync hooks
['the-top', 'willUpdate'],
['the-top', 'willRender'],

['the-middle', 'willUpdate'],
['the-middle', 'willRender']
]
nonInteractive: []
});

this.runTask(() => this.components['the-top'].rerender());
Expand All @@ -433,12 +425,7 @@ class LifeCycleHooksTest extends RenderingTest {
['the-top', 'didRender']
],

nonInteractive: [
// Sync hooks

['the-top', 'willUpdate'],
['the-top', 'willRender']
]
nonInteractive: []
});

this.runTask(() => set(this.context, 'twitter', '@horsetomdale'));
Expand Down Expand Up @@ -474,10 +461,7 @@ class LifeCycleHooksTest extends RenderingTest {
nonInteractive: [
// Sync hooks
['the-top', 'didUpdateAttrs', topAttrs],
['the-top', 'didReceiveAttrs', topAttrs],

['the-top', 'willUpdate'],
['the-top', 'willRender']
['the-top', 'didReceiveAttrs', topAttrs]
]
});

Expand Down Expand Up @@ -597,22 +581,18 @@ class LifeCycleHooksTest extends RenderingTest {
['the-parent', 'init'],
['the-parent', 'didInitAttrs', { attrs: parentAttrs }],
['the-parent', 'didReceiveAttrs', { newAttrs: parentAttrs }],
['the-parent', 'willRender'],

['the-first-child', 'init'],
['the-first-child', 'didInitAttrs', { attrs: firstAttrs }],
['the-first-child', 'didReceiveAttrs', { newAttrs: firstAttrs }],
['the-first-child', 'willRender'],

['the-second-child', 'init'],
['the-second-child', 'didInitAttrs', { attrs: secondAttrs }],
['the-second-child', 'didReceiveAttrs', { newAttrs: secondAttrs }],
['the-second-child', 'willRender'],

['the-last-child', 'init'],
['the-last-child', 'didInitAttrs', { attrs: lastAttrs }],
['the-last-child', 'didReceiveAttrs', { newAttrs: lastAttrs }],
['the-last-child', 'willRender']
['the-last-child', 'didReceiveAttrs', { newAttrs: lastAttrs }]
]
});

Expand Down Expand Up @@ -641,15 +621,7 @@ class LifeCycleHooksTest extends RenderingTest {
['the-parent', 'didRender']
],

nonInteractive: [
// Sync hooks

['the-parent', 'willUpdate'],
['the-parent', 'willRender'],

['the-first-child', 'willUpdate'],
['the-first-child', 'willRender']
]
nonInteractive: []
});

this.runTask(() => this.components['the-second-child'].rerender());
Expand Down Expand Up @@ -677,15 +649,7 @@ class LifeCycleHooksTest extends RenderingTest {
['the-parent', 'didRender']
],

nonInteractive: [
// Sync hooks

['the-parent', 'willUpdate'],
['the-parent', 'willRender'],

['the-second-child', 'willUpdate'],
['the-second-child', 'willRender']
]
nonInteractive: []
});

this.runTask(() => this.components['the-last-child'].rerender());
Expand Down Expand Up @@ -713,15 +677,7 @@ class LifeCycleHooksTest extends RenderingTest {
['the-parent', 'didRender']
],

nonInteractive: [
// Sync hooks

['the-parent', 'willUpdate'],
['the-parent', 'willRender'],

['the-last-child', 'willUpdate'],
['the-last-child', 'willRender']
]
nonInteractive: []
});

this.runTask(() => this.components['the-parent'].rerender());
Expand All @@ -743,12 +699,7 @@ class LifeCycleHooksTest extends RenderingTest {
['the-parent', 'didRender']
],

nonInteractive: [
// Sync hooks

['the-parent', 'willUpdate'],
['the-parent', 'willRender']
]
nonInteractive: []
});

this.runTask(() => setProperties(this.context, {
Expand Down Expand Up @@ -818,26 +769,14 @@ class LifeCycleHooksTest extends RenderingTest {
['the-parent', 'didUpdateAttrs', parentAttrs],
['the-parent', 'didReceiveAttrs', parentAttrs],

['the-parent', 'willUpdate'],
['the-parent', 'willRender'],

['the-first-child', 'didUpdateAttrs', firstAttrs],
['the-first-child', 'didReceiveAttrs', firstAttrs],

['the-first-child', 'willUpdate'],
['the-first-child', 'willRender'],

['the-second-child', 'didUpdateAttrs', secondAttrs],
['the-second-child', 'didReceiveAttrs', secondAttrs],

['the-second-child', 'willUpdate'],
['the-second-child', 'willRender'],

['the-last-child', 'didUpdateAttrs', lastAttrs],
['the-last-child', 'didReceiveAttrs', lastAttrs],

['the-last-child', 'willUpdate'],
['the-last-child', 'willRender']
['the-last-child', 'didReceiveAttrs', lastAttrs]
]
});

Expand Down Expand Up @@ -950,17 +889,14 @@ class LifeCycleHooksTest extends RenderingTest {
['the-top', 'init'],
['the-top', 'didInitAttrs', { attrs: topAttrs }],
['the-top', 'didReceiveAttrs', { newAttrs: topAttrs }],
['the-top', 'willRender'],

['the-middle', 'init'],
['the-middle', 'didInitAttrs', { attrs: middleAttrs }],
['the-middle', 'didReceiveAttrs', { newAttrs: middleAttrs }],
['the-middle', 'willRender'],

['the-bottom', 'init'],
['the-bottom', 'didInitAttrs', { attrs: bottomAttrs }],
['the-bottom', 'didReceiveAttrs', { newAttrs: bottomAttrs }],
['the-bottom', 'willRender']
['the-bottom', 'didReceiveAttrs', { newAttrs: bottomAttrs }]
]
});

Expand Down Expand Up @@ -1017,20 +953,11 @@ class LifeCycleHooksTest extends RenderingTest {
['the-top', 'didUpdateAttrs', topAttrs],
['the-top', 'didReceiveAttrs', topAttrs],

['the-top', 'willUpdate'],
['the-top', 'willRender'],

['the-middle', 'didUpdateAttrs', middleAttrs],
['the-middle', 'didReceiveAttrs', middleAttrs],

['the-middle', 'willUpdate'],
['the-middle', 'willRender'],

['the-bottom', 'didUpdateAttrs', bottomAttrs],
['the-bottom', 'didReceiveAttrs', bottomAttrs],

['the-bottom', 'willUpdate'],
['the-bottom', 'willRender']
['the-bottom', 'didReceiveAttrs', bottomAttrs]
]
});

Expand Down Expand Up @@ -1110,20 +1037,24 @@ class LifeCycleHooksTest extends RenderingTest {
let ret = [
['an-item', 'init'],
['an-item', 'didInitAttrs', { attrs: { count } }],
['an-item', 'didReceiveAttrs', { newAttrs: { count } }],
['an-item', 'willRender']
['an-item', 'didReceiveAttrs', { newAttrs: { count } }]
];
if (this.isInteractive) {
ret.push(['an-item', 'willInsertElement']);
ret.push(
['an-item', 'willRender'],
['an-item', 'willInsertElement']
);
}
ret.push(
['nested-item', 'init'],
['nested-item', 'didInitAttrs', { attrs: { } }],
['nested-item', 'didReceiveAttrs', { newAttrs: { } }],
['nested-item', 'willRender']
['nested-item', 'didReceiveAttrs', { newAttrs: { } }]
);
if (this.isInteractive) {
ret.push(['nested-item', 'willInsertElement']);
ret.push(
['nested-item', 'willRender'],
['nested-item', 'willInsertElement']
);
}
return ret;
};
Expand All @@ -1137,7 +1068,7 @@ class LifeCycleHooksTest extends RenderingTest {
['an-item', 'didRender']
];
} else {
return [ ];
return [];
}
};

Expand Down Expand Up @@ -1261,12 +1192,10 @@ class LifeCycleHooksTest extends RenderingTest {
['no-items', 'init'],
['no-items', 'didInitAttrs', { attrs: { } }],
['no-items', 'didReceiveAttrs', { newAttrs: { } }],
['no-items', 'willRender'],

['nested-item', 'init'],
['nested-item', 'didInitAttrs', { attrs: { } }],
['nested-item', 'didReceiveAttrs', { newAttrs: { } }],
['nested-item', 'willRender'],

['an-item', 'willDestroy'],
['nested-item', 'willDestroy'],
Expand Down
Loading

0 comments on commit 4c42b0d

Please sign in to comment.