Skip to content
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

Rename willRender to willUpdate since it's related to shouldUpdate and move implementation to renderer #285

Merged
merged 6 commits into from
Nov 2, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 67 additions & 0 deletions packages/metal-component/src/Component.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,28 @@ import { EventEmitter, EventHandler } from 'metal-events';
* rendered() {
* }
*
* willAttach() {
* }
*
* attached() {
* }
*
* willReceiveState() {
* }
*
* // willReceiveProps is only available in JSX components
* willReceiveProps() {
* }
*
* shouldUpdate() {
* }
*
* willUpdate() {
* }
*
* willDetach() {
* }
*
* detached() {
* }
*
Expand Down Expand Up @@ -132,6 +151,7 @@ class Component extends EventEmitter {
this.setUpDataManager_();
this.setUpSyncUpdates_();

this.on('stateWillChange', this.handleStateWillChange_);
this.on('stateChanged', this.handleComponentStateChanged_);
this.on('eventsChanged', this.onEventsChanged_);
this.addListenersFromObj_(this.dataManager_.get(this, 'events'));
Expand Down Expand Up @@ -179,6 +199,8 @@ class Component extends EventEmitter {
*/
attach(opt_parentElement, opt_siblingElement) {
if (!this.inDocument) {
this.emit('willAttach');
this.willAttach();
this.attachElement(opt_parentElement, opt_siblingElement);
this.inDocument = true;
this.attachData_ = {
Expand Down Expand Up @@ -245,6 +267,8 @@ class Component extends EventEmitter {
*/
detach() {
if (this.inDocument) {
this.emit('willDetach');
this.willDetach();
if (this.element && this.element.parentNode) {
this.element.parentNode.removeChild(this.element);
}
Expand Down Expand Up @@ -396,6 +420,16 @@ class Component extends EventEmitter {
});
}

/**
* Fires before state batch changes. Provides hook point for modifying
* state.
* @param {Event} event
* @protected
*/
handleStateWillChange_(event) {
this.willReceiveState(event.changes);
}

/**
* Checks if this component has sync updates enabled.
* @return {boolean}
Expand All @@ -422,6 +456,15 @@ class Component extends EventEmitter {
this.emit('rendered', firstRender);
}

/**
* Informs the component that the renderer is about to update. Calls the
* component's `willUpdate` lifecycle method.
* @param {Object} changes
*/
informWillUpdate(...args) {
this.willUpdate(...args);
}

/**
* Checks if the given function is a component constructor.
* @param {!function()} fn Any function
Expand Down Expand Up @@ -679,6 +722,30 @@ class Component extends EventEmitter {
validatorEventsFn_(val) {
return !isDefAndNotNull(val) || isObject(val);
}

/**
* Lifecycle. Fires before the component has been attached to the DOM.
*/
willAttach() {}

/**
* Lifecycle. Fires before component is detached from the DOM.
*/
willDetach() {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also have an empty placeholder for willUpdate? I see we are actually checking down the line to see if component.willUpdate exists, but it feels a bit inconsistent with how we are treating every other lifecycle method.

The most similar case seems to be rendered, which is actually always used from the component instance informRendered wrapper method...


/**
* Lifecycle. Called when the component is about to receive state changes.
* Provides a hook point for modifying state that can be used in the next
* rerender.
* @param {Object} changes Changes made to this.state
*/
willReceiveState() {}

/**
* Lifecycle. Called when the component's renderer is about to update.
* @param {Object} changes
*/
willUpdate() {}
}

/**
Expand Down
131 changes: 131 additions & 0 deletions packages/metal-component/test/Component.js
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,29 @@ describe('Component', function() {
assert.ok(comp.inDocument);
});

it('should run "willAttach" lifecycle method when the component is about to attach', function() {
class TestComponent extends Component {
}
sinon.spy(TestComponent.prototype, 'willAttach');
comp = new TestComponent();

assert.ok(comp.willAttach.calledBefore(Component.prototype.attached));
assert.strictEqual(1, comp.willAttach.callCount);
});

it('should emit "willAttach" lifecycle event when the component is about to attach', function() {
var listener = sinon.stub();
class TestComponent extends Component {
created() {
this.on('willAttach', listener);
}
}
comp = new TestComponent();

assert.ok(listener.calledBefore(Component.prototype.attached));
assert.strictEqual(1, listener.callCount);
});

it('should emit "attached" event when component is attached', function() {
comp = new Component({}, false);
var listener = sinon.stub();
Expand Down Expand Up @@ -142,6 +165,37 @@ describe('Component', function() {
assert.strictEqual(comp, comp.attach());
});

it('should run "willDetach" lifecycle method when the component is about to detach', function() {
class TestComponent extends Component {
}
sinon.spy(TestComponent.prototype, 'willDetach');
comp = new TestComponent();

assert.strictEqual(0, comp.willDetach.callCount);

comp.detach();

assert.ok(comp.willDetach.calledBefore(Component.prototype.detached));
assert.strictEqual(1, comp.willDetach.callCount);
});

it('should emit "willDetach" lifecycle event when the component is about to detach', function() {
var listener = sinon.stub();
class TestComponent extends Component {
created() {
this.on('willDetach', listener);
}
}
comp = new TestComponent();

assert.strictEqual(0, listener.callCount);

comp.detach();

assert.ok(listener.calledBefore(Component.prototype.detached));
assert.strictEqual(1, listener.callCount);
});

it('should dispose component', function() {
comp = new Component();

Expand Down Expand Up @@ -480,6 +534,83 @@ describe('Component', function() {
new CustomComponent();
});
});

it('should allow changes to state in "willReceiveState" without triggering multiple renders', function(done) {
const renderStub = sinon.stub();
let count = 0;

class CustomRenderer extends ComponentRenderer.constructor {
update(component) {
renderStub();

component.element.innerHTML = `${component.bar}:${component.foo}`;
component.informRendered();
}
}

class TestComponent extends Component {
willReceiveState(changes) {
this.foo = 'foo' + count;

count++;
}
}
TestComponent.STATE = {
bar: {
value: 'bar'
},

foo: {
value: 'foo'
}
};
TestComponent.RENDERER = new CustomRenderer();

comp = new TestComponent();

comp.bar = 'bar2';
comp.once('rendered', function() {
assert.equal(comp.element.innerHTML, 'bar2:foo0');

async.nextTick(function() {
comp.bar = 'bar3';
comp.once('rendered', function() {
assert.equal(renderStub.callCount, 2);
assert.equal(comp.element.innerHTML, 'bar3:foo1');

done();
});
});
});
});

it('should pass changed state data to "willReceiveState" method', function(done) {
class TestComponent extends Component {
}
TestComponent.prototype.willReceiveState = sinon.stub();
TestComponent.STATE = {
foo: {
value: 'foo'
}
};

comp = new TestComponent();

comp.foo = 'foo2';

async.nextTick(function() {
assert.equal(comp.willReceiveState.callCount, 1);
assert.deepEqual(comp.willReceiveState.args[0][0], {
foo: {
key: 'foo',
newVal: 'foo2',
prevVal: 'foo'
}
});

done();
});
});
});

describe('Render', function() {
Expand Down
24 changes: 20 additions & 4 deletions packages/metal-incremental-dom/src/IncrementalDomRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -154,10 +154,14 @@ class IncrementalDomRenderer extends ComponentRenderer.constructor {
* @param {!Component} component
*/
renderInsidePatch(component) {
const changes = getChanges(component);

const shouldRender = !component.wasRendered ||
this.shouldUpdate(component, getChanges(component)) ||
this.shouldUpdate(component, changes) ||
IncrementalDOM.currentPointer() !== component.element;
if (shouldRender) {
this.willUpdate_(component, changes);

render(component);
} else if (component.element) {
this.skipRender();
Expand Down Expand Up @@ -219,12 +223,24 @@ class IncrementalDomRenderer extends ComponentRenderer.constructor {
* @param {Object} data
*/
update(component, data) {
if (data.forceUpdate ||
this.shouldUpdate(component, getChanges(component))) {

const changes = getChanges(component);
if (data.forceUpdate || this.shouldUpdate(component, changes)) {
this.willUpdate_(component, changes);
this.patch(component);
}
}

/**
* Invokes component's "willUpdate" lifecycle method if applicable.
* @param {!Component} component
* @param {Object} changes
*/
willUpdate_(component, changes) {
if (!component.wasRendered || !changes) {
return;
}
component.informWillUpdate(...this.buildShouldUpdateArgs(changes));
}
}

const renderer = new IncrementalDomRenderer();
Expand Down
Loading