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

Improving Metal.js Lifecycle and Compatiblilty #255

Closed
mthadley opened this issue Sep 7, 2017 · 11 comments
Closed

Improving Metal.js Lifecycle and Compatiblilty #255

mthadley opened this issue Sep 7, 2017 · 11 comments

Comments

@mthadley
Copy link

mthadley commented Sep 7, 2017

Now that Metal.js is more mature, we can start thinking about both improving it's lifecycle features and also making it more compatible with knowledge from other existing frameworks. In particular, we want to look at moving over to lifecycle names that are also found in React. This would be similar to the relationship between preact and React.

So as an example, we could either alias or rename attached to componentDidMount. Or we could add new lifecycles, like componentWillRecieveProps.

@jbalsas @bryceosterhaus @eduardolundgren Moving this conversation here.

@jbalsas
Copy link
Contributor

jbalsas commented Sep 8, 2017

Hey @mthadley, I've tried to gather some information here to see what we have and what we might need. Could you double check and expand on this?

React.js Metal.js
Mounting constructor constructor
render render 
componentWillMount ⚠️
componentDidMount attached
Updating componentWillReceiveProps ⚠️
shouldComponentUpdate shouldUpdate
componentWillUpdate ⚠️
componentDidUpdate rendered
Unmounting componentWillUnmount ⚠️
APIs setState setState
forceUpdate ⚠️
Class Properties defaultProps  ⚠️
displayName ⚠️
Instance Properties props props
state state

@mthadley
Copy link
Author

mthadley commented Sep 8, 2017

I believe sync[AttrName] methods also fit in somewhere here. They seem to be like a more specific componentDidUpdate since they run after a specific state attribute changed.

@yuchi
Copy link
Contributor

yuchi commented Sep 9, 2017

Are you proposing a [attr]DidChange?

@bryceosterhaus
Copy link
Member

For most use cases, I think by having both componentWillReceiveProps and componentDidUpdate satisfies any logic you might do in sync[AttrName].

@robframpton
Copy link

Hey guys,

I think the biggest question I have regarding this. Is whether we need to switch
to React's naming patterns for lifecycle methods, or stick with our current
pattern and not worry about making it the same as React.

1) One to one match with React and backwards compatibilty:

In this scenario we adopt the method names from React, and also keep around the
current lifecycle methods for backwards compatibilty.

  React.js Metal.js
Mounting constructor constructor
  render render
  componentWillMount componentWillMount
  componentDidMount componentDidMount / attached
Updating componentWillReceiveProps componentWillReceiveProps
  shouldComponentUpdate shouldComponentUpdate / shouldUpdate
  componentWillUpdate componentWillUpdate
  componentDidUpdate componentDidUpdate / rendered
Unmounting componentWillUnmount componentWillUnmount

2) Custom names (sticking with our current naming pattern):

In this scenario, we just stick with our current lifecycle methods, and name the
new methods in a similar fashion.

  React.js Metal.js
Mounting constructor constructor
  render render
  componentWillMount willAttach
  componentDidMount attached
Updating componentWillReceiveProps willReceiveProps
  shouldComponentUpdate shouldUpdate
  componentWillUpdate willRender
  componentDidUpdate rendered
Unmounting componentWillUnmount willDetach

Recently our stance on Metal is that it's primarily intended to fit our needs
internally. If that's the case then option 2 actually makes more sense to me,
but I'm open to dicussion.

Thoughts? @jbalsas @mthadley @bryceosterhaus

@bryceosterhaus
Copy link
Member

Recently our stance on Metal is that it's primarily intended to fit our needs
internally.

I agree, option 2 makes most sense to me. I think its best for us to just keep our own naming convention and not follow React, otherwise we will have a tendency to try and mirror them as much as possible. It makes sense for Preact, but not for us.

@mthadley
Copy link
Author

mthadley commented Oct 4, 2017

I'm on board with option 2. I think the biggest problem was not having equivalent lifecycles for specific purposes (like componentWillRecieveProps). Instead of renaming all of our methods, the best thing we can do for the framework is to just increase the amount of documentation. 📓

@brunobasto
Copy link

brunobasto commented Oct 4, 2017

I'm fine going either way. The strongest argument I see that could back up option 1 is that it would decrease a couple of degrees on the learning curve steepness. But, like @mthadley said, that could be improved by having plentiful and easy to find documentation.

@robframpton
Copy link

I've sent #270. I'm still open to renaming the new lifecycle methods if anyone has better ideas.

The strangest part about it, is that Soy components have willReceiveState, and JSX components have willReceiveProps, but if there is something more general we can call it that would be good.

The problem is that for JSX components the method won't fire for internal state changes (only props), so we can't just call it willReceiveState across the board.

robframpton pushed a commit to robframpton/metaljs.com that referenced this issue Oct 6, 2017
@robframpton
Copy link

Also related: #271

Adds forceUpdate to Component.

@jbalsas
Copy link
Contributor

jbalsas commented Nov 2, 2017

Changes have been merged to develop and will be released soon as part of 2.14.0

@jbalsas jbalsas closed this as completed Nov 2, 2017
jbalsas pushed a commit to bryceosterhaus/metaljs.com that referenced this issue Nov 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants