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

robframpton · 2017-10-23T20:05:32Z

No description provided.

robframpton · 2017-11-01T21:48:29Z

Just noticed this has merge conflicts. I'll rebase.

…nent

…oint

…ent and JSXComponent respectively

jbalsas · 2017-11-01T21:50:44Z

hey @Robert-Frampton, I'm finishing the review, hold on if you wish so you can do all at once!

…d move implementation to renderer

jbalsas

Hey @Robert-Frampton, looks good to me in general, just some minor remarks and the rebase and we should be ready to go! 👍

jbalsas · 2017-10-24T07:08:25Z

packages/metal-component/src/Component.js

@@ -28,12 +28,21 @@ import { EventEmitter, EventHandler } from 'metal-events';
 *   created() {
 *   }
 *
+ *   willRender() {


Should this be willUpdate?

jbalsas · 2017-11-01T21:47:31Z

packages/metal-component/src/Component.js

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


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...

jbalsas · 2017-11-01T21:48:00Z

packages/metal-component/test/Component.js

@@ -94,6 +94,29 @@ describe('Component', function() {
 			assert.ok(comp.inDocument);
 		});

+		it('should run "willAttach" lifecycle method when the component about to attach', function() {


Missing verb? when the component **is** about to attach?

jbalsas · 2017-11-01T21:49:18Z

packages/metal-component/test/Component.js

+			assert.strictEqual(1, comp.willAttach.callCount);
+		});
+
+		it('should emit "willAttach" lifecycle method when the component about to attach', function() {


Are we saying emit ... method in the rest of tests? maybe better to say emit ... lifecycle **event** when the component **is** about to...?

jbalsas · 2017-11-01T21:49:57Z

packages/metal-component/test/Component.js

@@ -142,6 +165,37 @@ describe('Component', function() {
 			assert.strictEqual(comp, comp.attach());
 		});

+		it('should run "willDetach" lifecycle method when the component about to detach', function() {


Missing verb? when the component is about to detach?

jbalsas · 2017-11-01T21:50:10Z

packages/metal-component/test/Component.js

+			assert.strictEqual(1, comp.willDetach.callCount);
+		});
+
+		it('should emit "willDetach" lifecycle method when the component about to detach', function() {


Are we saying emit ... method in the rest of tests? maybe better to say emit ... lifecycle event when the component is about to...?

jbalsas · 2017-11-01T21:53:58Z

packages/metal-incremental-dom/src/IncrementalDomRenderer.js

+	 * @param {Object} changes
+	 */
+	willUpdate_(component, changes) {
+		if (!component.willUpdate || !component.wasRendered || !changes) {


See comment above... if we want to follow the render approach, we could provide a wrapping API for it like component.informWillUpdate. I'm fine keeping it here too :)

Also, from the calling code, it might not be needed to check for !changes, since it is always invoked within something that used shouldUpdate(..., changes) which already does that check?

Hey @jbalsas

In that case we might want to do the same for shouldUpdate, right now it just checks for a component.shouldUpdate method from the renderer. What do you think?

Yup, we probably should! 👍

jbalsas · 2017-11-01T21:58:34Z

packages/metal-jsx/src/JSXComponent.js

+	 */
+	handleStateWillChange_(event) {
+		if (event.type !== 'state' && this.willReceiveProps) {
+			this.willReceiveProps(event.changes);


This is only for JSX, right? We still should document it somehow in the docs, as we do for the other lifecycle methods. I know we are likely updating the site docs with this, but it is fairly easy to miss on code inspection.

Also, same remarks as willUpdate about providing a default empty placeholder.

jbalsas · 2017-11-01T22:01:36Z

packages/metal-state/src/State.js

@@ -304,6 +304,8 @@ class State extends EventEmitter {
 	 */
 	emitBatchEvent_() {
 		if (!this.isDisposed()) {
+			this.context_.emit('stateWillChange', this.scheduledBatchData_);


Not related to this change in particular, but I'm wondering how can we document what events are emitted so one knows what to listen :)

robframpton · 2017-11-02T15:58:23Z

Hey @jbalsas

I made most of the changes you mentioned. I decided against adding an informShouldUpdate method and placeholder shouldUpdate method.

This is because it would be the only lifecycle method that needs to return a value, it just felt weird doing this...

informShouldUpdate(...args) {
  return this.shouldUpdate(...args);
}

shouldUpdate() {
  return true;
}

None of the other inform... methods need to return anything back to the renderer, at that point in the contract the renderer is just informing the component that something happened and doesn't care what the component does.

We can still do it if you think this discrepancy is fine.

Edit: I should also note that if we add a placeholder that we'll have to change this logic in the Soy component.

jbalsas

Hey @Robert-Frampton, looks good, just 2 final issues, (sorry for the nit-picking) and we're good to go!

No need to get crazy about the forceUpdate placeholder, let's see how this works and we can iterate later.

jbalsas · 2017-11-02T16:02:20Z

packages/metal-component/src/Component.js

@@ -27,13 +27,32 @@ import { EventEmitter, EventHandler } from 'metal-events';
 * class CustomComponent extends Component {
 *   created() {
 *   }
- *
+


Unintended deletion?

jbalsas · 2017-11-02T16:04:07Z

packages/metal-component/src/Component.js

+ *   willReceiveState() {
+ *   }
+ *
+ *   // JSX components only


This comment here is a bit confusing... for a while I assumed everything below this point was related to JSX components only and not just this method... maybe place the comment inside? or make it more explicit, like willReceiveProps is only available in JSX components?

robframpton · 2017-11-02T16:10:21Z

Sorry, what exactly do you mean in regards to a forceUpdate placeholder?

Edit: oh, I'm guessing you meant shouldUpdate ;)

…aceholder for `shouldUpdate` as some renderers need to check if it exists)

jbalsas · 2017-11-02T16:11:52Z

Sorry, I meant shouldUpdate. It was related to your previous comment. Sounds good to me 👍

robframpton · 2017-11-02T16:35:11Z

Updated.

robframpton force-pushed the lifecycle branch 2 times, most recently from ceeb7df to ff9fcc9 Compare October 23, 2017 20:06

robframpton mentioned this pull request Oct 23, 2017

Add willReceiveState and willReceiveProps lifecycle methods to Component and JSXComponent respectively #270

Closed

jbalsas mentioned this pull request Oct 27, 2017

Document lifecycle changes. Documents metal/metal.js#255 bryceosterhaus/metaljs.com#71

Merged

jbalsas added this to the 2.14.0 milestone Nov 1, 2017

Robert-Frampton added 3 commits November 1, 2017 14:49

Add willRender, willAttach, and willDetach lifecycle methods to Compo…

2b9d6bc

…nent

Emit stateWillChange event from state instance for providing a hook p…

f527c41

…oint

Add willReceiveState and willReceiveProps lifecycle methods to Compon…

f08950c

…ent and JSXComponent respectively

Rename willRender to willUpdate since it's related to shouldUpdate an…

5425101

…d move implementation to renderer

jbalsas suggested changes Nov 1, 2017

View reviewed changes

Fix JSDoc and test description typos

ba375c9

robframpton force-pushed the lifecycle branch from ff9fcc9 to 8ae43fb Compare November 2, 2017 15:46

jbalsas suggested changes Nov 2, 2017

View reviewed changes

Provide placeholder functions for lifecycle methods (can't provide pl…

7937ce6

…aceholder for `shouldUpdate` as some renderers need to check if it exists)

robframpton force-pushed the lifecycle branch from 8ae43fb to 7937ce6 Compare November 2, 2017 16:12

jbalsas approved these changes Nov 2, 2017

View reviewed changes

jbalsas merged commit 35eb911 into metal:develop Nov 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

robframpton commented Oct 23, 2017

robframpton commented Nov 1, 2017

jbalsas commented Nov 1, 2017

jbalsas left a comment

jbalsas Oct 24, 2017

jbalsas Nov 1, 2017

jbalsas Nov 1, 2017

jbalsas Nov 1, 2017

jbalsas Nov 1, 2017

jbalsas Nov 1, 2017

jbalsas Nov 1, 2017 •

edited

Loading

jbalsas Nov 1, 2017

robframpton Nov 1, 2017

jbalsas Nov 1, 2017

jbalsas Nov 1, 2017

jbalsas Nov 1, 2017

robframpton commented Nov 2, 2017 •

edited

Loading

jbalsas left a comment

jbalsas Nov 2, 2017

jbalsas Nov 2, 2017

robframpton commented Nov 2, 2017 •

edited

Loading

jbalsas commented Nov 2, 2017

robframpton commented Nov 2, 2017

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

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

Conversation

robframpton commented Oct 23, 2017

robframpton commented Nov 1, 2017

jbalsas commented Nov 1, 2017

jbalsas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbalsas Nov 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robframpton commented Nov 2, 2017 • edited Loading

jbalsas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robframpton commented Nov 2, 2017 • edited Loading

jbalsas commented Nov 2, 2017

robframpton commented Nov 2, 2017

jbalsas Nov 1, 2017 •

edited

Loading

robframpton commented Nov 2, 2017 •

edited

Loading

robframpton commented Nov 2, 2017 •

edited

Loading