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

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

Closed
wants to merge 4 commits into from

Conversation

robframpton
Copy link

@robframpton robframpton commented Oct 6, 2017

See #255

@robframpton robframpton changed the base branch from master to develop October 10, 2017 16:25
@jbalsas jbalsas added this to the 2.14.0 milestone Oct 11, 2017
Copy link
Contributor

@jbalsas jbalsas left a comment

Choose a reason for hiding this comment

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

Hey @Robert-Frampton, I've added a couple of notes about willAttach and willRender. Can you take a look at those?

Also, I kinda added a merge commit here by mistake. Could you send a new PR once you're ready? 👼

Thanks!

@@ -172,6 +182,8 @@ class Component extends EventEmitter {
*/
attach(opt_parentElement, opt_siblingElement) {
if (!this.inDocument) {
this.emit('willAttach');
this.willAttach();
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @Robert-Frampton, based on componentWillMount():

componentWillMount() is invoked immediately before mounting occurs. It is called before render(), therefore setting state synchronously in this method will not trigger a re-rendering. Avoid introducing any side-effects or subscriptions in this method.

Can we assume the same with this?

Copy link
Author

Choose a reason for hiding this comment

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

Hey @jbalsas

So an area where Metal is already different than React is that our render actually happens before attach. This is how the lifecycle is ordered on first render.

created > willRender > rendered > willAttach > attached > willDetach > detached > disposed

I think the only way we can mirror React completely is to do a breaking change and change the order so that attachment happens before render, but obviously we can't do that for 2.14. Although we could move willAttach only, but it feels weird to me.

created > willAttach > willRender > rendered > attached > willDetach > detached > disposed.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine for now... I think we are entitled to some little quirks of our own ;)

I think we just need to improve our Component Lifecycle documentation so everything is clear :)

@@ -501,6 +527,9 @@ class Component extends EventEmitter {
* be called manually later to actually attach it to the dom.
*/
renderComponent(opt_parentElement) {
const firstRender = !this.hasRendererRendered_;
this.emit('willRender', firstRender);
this.willRender(firstRender);
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on componentWillUpdate(), the signature for this method should be:

componentWillUpdate(nextProps, nextState)

Furthermore:

componentWillUpdate() is invoked immediately before rendering when new props or state are being received. Use this as an opportunity to perform preparation before an update occurs. This method is not called for the initial render.

Finally, do we have the last note in mind?

componentWillUpdate() will not be invoked if shouldComponentUpdate() returns false.

Copy link
Author

Choose a reason for hiding this comment

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

I update this in the new pull. I changed the name to willUpdate since it's closely related to shouldUpdate and receives the same arguments.

@robframpton
Copy link
Author

#285

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants