-
Notifications
You must be signed in to change notification settings - Fork 839
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
Update EuiInMemoryTable's and EuiCustomItemAction lifecycle for React 16.3 #859
Update EuiInMemoryTable's and EuiCustomItemAction lifecycle for React 16.3 #859
Conversation
fd1e361
to
a3f5d22
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⭐️ Thanks for tagging me on this, it's awesome seeing these changes being made! I had one nitpicky suggestion, feel free to ignore it.
I know that duplicating items
onto the state is the idiomatic way to address this kind of situation. I think one vulnerability of this pattern is that someone may see items
on state and think it can be manipulated internally, when really we're just caching it until the corresponding prop changes. Do you think it's worth thinking of a naming convention or other pattern we can reuse in the future to make it clear that this type of state property isn't meant to be changed?
For example, maybe prefixing it with an underscore, state._items
. Or naming it explicitly: propItems
. Or name-spacing it inside of an object: state.props.items
(I think I like this one the most). Just something that indicates that it isn't a traditional state property.
pageIndex: 0, | ||
}); | ||
}; | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit, but since we're exiting early on line 247, the else
isn't necessary.
static getDerivedStateFromProps(nextProps, prevState) {
if (nextProps.items !== prevState.items) {
// We have new items because an external search has completed, so reset pagination state.
return {
items: nextProps.items,
pageIndex: 0,
};
}
return null;
}
@cjcenizal I played with pre-computing the visible items and storing on state, but it gets repetitive as you must explicitly re-compute the items when the pagination or row length state values change. |
11b9cd7
to
c5ee0fc
Compare
And then React 16.4 was released last night which fixes that, so getDerivedStateFromProps is called after |
Haha well at least they fixed it quickly! It's pretty cool that their guidance is to store previous props as |
For #763
getDerivedStateFromProps
to replacecomponentWillReceiveProps
. Moveditems
into component state instead of accessing it from props.componentWillMount
->componentDidMount