fix: replace componentWillReceiveProps by getDerivedStateFromProps and componentDidUpdate #990

daynin · 2019-09-09T07:45:52Z

relates to: #988

n1ghtmare

I've left some comments, also, just FYI, I'm getting some errors in the console. Didn't see those before:

n1ghtmare · 2019-09-09T13:11:11Z

lib/ReactGridLayout.jsx

@@ -290,20 +301,20 @@ export default class ReactGridLayout extends React.Component<Props, State> {
    this.onLayoutMaybeChanged(this.state.layout, this.props.layout);
  }

-  componentWillReceiveProps(nextProps: Props) {
+  static getDerivedStateFromProps(nextProps: Props, prevState: State) {


There is a really big performance impact with those changes. I'm getting lag, since when we use getDerivedStateFromProps and componentDidUpdate it does the equality checks every single time. They're not equivalent to componentWillReceiveProps which is only triggered when props change (not state). Not sure how we can optimize this.

Also if we go this route, we can do the changes without getDerivedStateFromProps and just go with componentDidUpdate (with adding a check of what changed and trigger state changes only when needed).

I did some changes locally on my branch very similar to yours, but it impacts performance (maybe we can do some measurements and see how bad the impact actually is). Nevertheless, I think for such big changes it will be a good idea to involve @STRML and see what he thinks.

Yeah this is problematic, this whole block is going to run many times while dragging an item. We'll need to profile & brainstorm where we can optimize.

@STRML @n1ghtmare we can try this

Also if we go this route, we can do the changes without getDerivedStateFromProps and just go with componentDidUpdate

with props mirroring. It can help us to avoid extra comparisons

What do you think?

We may want to require the library user to send a new layout object (rather than mutating the existing one) or supply something optional like a layoutDidUpdate function of arity (newLayout: Layout, oldLayout: Layout) => boolean where the default implementation is (newLayout, oldLayout) => newLayout !== oldLayout.

This will help us avoid an expensive deep comparison.

Perhaps some of you know a way this is handled in other libraries? I don't want to require the author to mutate key, which will unmount all children.

I think that layout recreating on the user side is not an obvious way to fix it. I mean not obvious for a user

Maybe replacing the code into componentDidMount will be enough to fix performance issues. I'll try to compare cases with componentWillReceiveProps, getDerivedStateFromProps + componentDidUpdate and componentDidUpdate

@STRML Just for the sake of brainstorming (and I'm sure I'm missing something), but are we doing this comparison only for when the compactType changes or when an item (child) is added/removed?

STRML · 2019-09-09T15:43:06Z

lib/ReactGridLayout.jsx

@@ -85,6 +89,12 @@ export type Props = {
 };
 // End Types

+const compactType = (props: ?Object): CompactType => {


Type here is Props, it should not be optional and it should not be Object

STRML · 2019-09-09T15:45:05Z

lib/GridItem.jsx

    const { dragging } = this.state;

-    if (!droppingPosition || !this.props.droppingPosition) {
+    if (!droppingPosition || !prevProps.droppingPosition) {


TBH this bailout here makes this code hard to read. #980 should have refactored the droppable functionality into another function so this logic is more readable.

@STRML thank you for the comment! I'll fix it right after the performance issue

daynin · 2019-09-09T16:38:58Z

@n1ghtmare

I've left some comments, also, just FYI, I'm getting some errors in the console. Didn't see those before

I fixed it

daynin · 2019-09-12T13:40:38Z

Performance comparison:
master: http://s.csssr.ru/U286BQJEP/20190912141129.mp4
fix-cwrp-warnings (before fix): http://s.csssr.ru/U286BQJEP/20190912132331.mp4
fix-cwrp-warnings (after fix): http://s.csssr.ru/U286BQJEP/20190912141619.mp4

daynin · 2019-09-12T13:41:12Z

lib/GridItem.jsx

-        onStart={this.onDragHandler("onDragStart")}
-        onDrag={this.onDragHandler("onDrag")}
-        onStop={this.onDragHandler("onDragStop")}
+        onStart={this.onDragStart}


do not recreate new functions every single render cycle

daynin · 2019-09-12T13:43:43Z

lib/ReactGridLayout.jsx

    let newLayoutBase;
+
+    if (prevState.activeDrag) {


do not compare layouts while dragging

Dude, you've put a lot of effort in this. Great job. I love this approach. When I get some time, I'll do some testing and let you know.

@n1ghtmare thank you!

daynin · 2019-09-12T13:46:31Z

@STRML @n1ghtmare check it out, pls

STRML · 2019-09-12T21:53:45Z

CPU usage looks good. Nice work.

n1ghtmare

Great job dude! Much appreciated.

Can you check my comment and see what you think?

n1ghtmare · 2019-09-13T09:45:19Z

lib/ReactGridLayout.jsx

+  }
+
+  componentDidUpdate(prevProps: Props, prevState: State) {
+    const newLayout = this.state.layout;


This still triggers too many times (give it a shot with a console.log). Do you think we can solve it by changing it to the following:

if (!this.state.activeDrag) { const newLayout = this.state.layout; const oldLayout = prevState.layout; this.onLayoutMaybeChanged(newLayout, oldLayout); }

Similar to what you're doing in the getDerivedStateFromProps, or will this break something? I tried it locally and everything seems to be working just fine. However it should improve performance even more. What do you think?

@n1ghtmare done

@daynin From what I see, this change shouldn't break anything. Can you confirm this? Otherwise performance is great. @STRML Do you want to take a quick look at this, when you get some time?

That's a nice optimization. It shouldn't break anything as the grid should only move in two circumstances:

An active drag, or

An update to the layout property from the parent.

doesn't apply here so we should be okay 👍

componentDidUpdate - fix warnings - fix performance issues

n1ghtmare · 2019-09-14T17:15:21Z

@STRML Can you please change version and push this to npm when you get the chance?

hd4ng · 2019-12-27T00:01:36Z

@daynin @n1ghtmare The onLayoutChange is triggered 2 times when dragging an item outside of the container and drop it.
On version 0.16.6, it works correctly.
This is code sandbox demo: https://codesandbox.io/s/red-waterfall-og2gv
Could you have anything other workarounds for this? Thanks

daynin requested review from STRML and n1ghtmare September 9, 2019 07:45

daynin self-assigned this Sep 9, 2019

n1ghtmare requested changes Sep 9, 2019

View reviewed changes

STRML reviewed Sep 9, 2019

View reviewed changes

daynin force-pushed the fix-cwrp-warnings branch from a0e9ada to 6f72052 Compare September 9, 2019 16:38

daynin force-pushed the fix-cwrp-warnings branch from 6f72052 to 25ea554 Compare September 9, 2019 16:41

daynin added the WIP - Don't Merge label Sep 9, 2019

daynin mentioned this pull request Sep 11, 2019

feat(grid): add an ability to drop elements from outside of a grid #980

Merged

daynin force-pushed the fix-cwrp-warnings branch from 25ea554 to 729b890 Compare September 12, 2019 13:39

daynin commented Sep 12, 2019

View reviewed changes

daynin force-pushed the fix-cwrp-warnings branch from 729b890 to e92c4ee Compare September 12, 2019 13:45

daynin requested review from STRML and n1ghtmare September 12, 2019 13:46

daynin removed the WIP - Don't Merge label Sep 12, 2019

n1ghtmare reviewed Sep 13, 2019

View reviewed changes

fix: replace componentWillReceiveProps by getDerivedStateFromProps and

3d4e863

componentDidUpdate - fix warnings - fix performance issues

daynin force-pushed the fix-cwrp-warnings branch from e92c4ee to 3d4e863 Compare September 13, 2019 10:53

n1ghtmare self-requested a review September 13, 2019 11:14

n1ghtmare approved these changes Sep 13, 2019

View reviewed changes

daynin merged commit 9fe5882 into react-grid-layout:master Sep 14, 2019

daynin deleted the fix-cwrp-warnings branch September 14, 2019 17:10

daynin mentioned this pull request Sep 14, 2019

Console warnings using react 16.9 #988

Closed

STRML mentioned this pull request Oct 9, 2019

Layout not refreshed when the width is updated at the same time as the layouts #867

Closed

STRML added this to the 0.17.0 milestone Oct 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: replace componentWillReceiveProps by getDerivedStateFromProps and componentDidUpdate #990

fix: replace componentWillReceiveProps by getDerivedStateFromProps and componentDidUpdate #990

daynin commented Sep 9, 2019

n1ghtmare left a comment

n1ghtmare Sep 9, 2019

n1ghtmare Sep 9, 2019

STRML Sep 9, 2019

daynin Sep 9, 2019

STRML Sep 9, 2019

daynin Sep 9, 2019

n1ghtmare Sep 10, 2019

STRML Sep 9, 2019

STRML Sep 9, 2019

daynin Sep 9, 2019

daynin commented Sep 9, 2019

daynin commented Sep 12, 2019

daynin Sep 12, 2019

daynin Sep 12, 2019

n1ghtmare Sep 12, 2019

daynin Sep 12, 2019

daynin commented Sep 12, 2019

STRML commented Sep 12, 2019

n1ghtmare left a comment

n1ghtmare Sep 13, 2019 •

edited

Loading

daynin Sep 13, 2019

n1ghtmare Sep 13, 2019

STRML Sep 13, 2019

n1ghtmare commented Sep 14, 2019

hd4ng commented Dec 27, 2019 •

edited

Loading

fix: replace componentWillReceiveProps by getDerivedStateFromProps and componentDidUpdate #990

fix: replace componentWillReceiveProps by getDerivedStateFromProps and componentDidUpdate #990

Conversation

daynin commented Sep 9, 2019

n1ghtmare 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

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

daynin commented Sep 9, 2019

daynin commented Sep 12, 2019

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

daynin commented Sep 12, 2019

STRML commented Sep 12, 2019

n1ghtmare left a comment

Choose a reason for hiding this comment

n1ghtmare Sep 13, 2019 • 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

n1ghtmare commented Sep 14, 2019

hd4ng commented Dec 27, 2019 • edited Loading

n1ghtmare Sep 13, 2019 •

edited

Loading

hd4ng commented Dec 27, 2019 •

edited

Loading