Replace deprecated / async-unsafe lifecycles #1009

bvaughn · 2018-02-10T02:31:07Z

Note this PR updates all components (including examples) except for Grid. Grid was turning out to be a large update, so I reverted the changes to it in 72d7fd1 and we can migrate it in a follow up commit.

For all other (non-Grid components) this PR includes:

Replacing componentWillMount, componentWillReceiveProps, and componentWillUpdate with async-safe lifecycles in advance of React 16.4 deprecation warnings.
Using react-lifecycles-compat where necessary to ensure backwards and forwards compatibility with the new static getDerivedStateFromProps lifecycle.
Tidied up and replaced a few constructor-binded methods with arrow functions.

Relates to this RFC. In-progress reference documentation available here.

Resolves #1008

TrySound · 2018-02-10T16:55:39Z

source/WindowScroller/WindowScroller.js

-      unregisterScrollListener(this, scrollElement);
-      registerScrollListener(this, nextScrollElement);
+      // $FlowFixMe Why is this necessary? There's a check right above.
+      unregisterScrollListener(this, prevProps.scrollElement);


because updatePosition may change this value value.

Ah! Nice observation.

before is worked ok, because it was const instead of field

TrySound · 2018-02-10T16:57:50Z

source/WindowScroller/WindowScroller.js

    }
  }
-


why remove newline?

Probably an accident. This is a WIP PR. Not quite ready for nit-review yet 😉

(I'll do a final cleanup pass myself once I'm done with the major refactor)

bvaughn · 2018-02-11T22:49:48Z

Just a quick update- the bulk of the migration is done, but Grid triggers a warning due to a setState call during render. I'm in the process of fixing this locally, but it has a bigger footprint than I wish because it involves a lot of instance properties/methods.

Once I finish this bit (maybe not this weekend, perhaps tomorrow) I'll do a once-over on the other components to see if there's more tidying up I want to do.

bvaughn · 2018-02-12T01:26:40Z

At this point, I find myself wanting to make everything in Grid static, except for the lifecycles that are required to be instance methods- but even those, I think should just defer to static methods.

My initial attempt to only update things that need access to cached values on the instance feels like it's getting out of control. So many of the helper methods call each other.

It's unfortunate that there's so much complexity within the timing and sequence of things Grid does. I hope the current test coverage is sufficient to cover my ass with these changes.

Edit I guess this statement was a bit hyperbolic. I don't literally want to move everything off the instance. I should probably just step away for this for the evening and come back to it tomorrow.

bvaughn · 2018-02-13T02:49:41Z

Made some progress today but got distracted by another alpha release and some other work things. Will resume tomorrow!

wuweiweiwu · 2018-03-11T22:17:13Z

@bvaughn I can help migrate to getDerivedStateFromProps :)

Should I use the React 16.3(.0-alpha) when I'm developing locally?

bvaughn · 2018-03-12T15:37:23Z

I think the only component left to migrate is Grid and it's kind of in a halfway done state.

bvaughn · 2018-03-14T15:25:52Z

Since updates to Grid are a lot more involved than the other components, I've decided to revert the changes in this branch and just focus on landing the others. Grid can be updated (by me, or someone else) in a follow up PR. This way I'm not holding up so many things.

…atic lifecycle

bvaughn · 2018-03-14T15:46:31Z

This PR is now ready for review and testing!

wuweiweiwu

LGTM! :) Just little nits

wuweiweiwu · 2018-03-15T00:20:30Z

source/Masonry/Masonry.js

@@ -328,11 +332,11 @@ export default class Masonry extends React.PureComponent<Props> {
    );
  }

-  _debounceResetIsScrollingCallback() {
+  _debounceResetIsScrollingCallback = () => {


nit: some functions are arrows and some are not.

It's actually intentional. I think I might have a lint rule to protect against unnecessary arrow functions. Not sure though. But I added arrow functions where I wanted to ensure the correct "this" even after passing references around

wuweiweiwu · 2018-03-15T00:22:20Z

source/Collection/CollectionView.js

+    ) {
+      return {
+        scrollLeft:
+          nextProps.scrollLeft != null


nit: !== unless nextProps only contains updated props? since undefined == null

This check handles null and undefined. It's probably not necessary but I'd prefer to be safe

Ok that makes sense.

wuweiweiwu · 2018-03-15T00:34:15Z

source/CellMeasurer/CellMeasurer.DynamicHeightTableColumn.example.js

@@ -88,11 +82,11 @@ export default class DynamicHeightTableColumn extends React.PureComponent {
        </div>
      </CellMeasurer>
    );
-  }
+  };


nit: semi colon inconsistencies

Prettier formats everything 🙂

wuweiweiwu · 2018-03-15T00:35:02Z

source/WindowScroller/WindowScroller.js

-  componentWillReceiveProps(nextProps: Props) {
-    const scrollElement = this.props.scrollElement;
-    const nextScrollElement = nextProps.scrollElement;
+  componentDidUpdate(prevProps: Props, prevState: State) {


do we need to include prevState if we're not using it?

I guess we don't need to but it's part of the lifecycles signature so it's just habit to add it. 😁

bvaughn · 2018-03-15T01:38:39Z

Thanks for the review @wuweiweiwu! I added a couple of responses inline. They're a bit terse because I'm on my mobile

TrySound · 2018-03-15T09:36:02Z

source/MultiGrid/MultiGrid.js

@@ -644,7 +632,6 @@ export default class MultiGrid extends React.PureComponent {
        ref={this._bottomLeftGridRef}
        rowCount={Math.max(0, rowCount - fixedRowCount) + additionalRowCount}
        rowHeight={this._rowHeightBottomGrid}
-        scrollTop={scrollTop}


Looks like it may have been an accident. Good catch.

Oh, no never mind. I just removed the explicit scrollTop={scrollTop} because it wasn't necessary. I'm already spreading {...props} above. 😄

The other props are explicitly passed because they have different names.

TrySound · 2018-03-15T09:37:50Z

I like how much cleaner became code in a few places.

timhwang21

Apologies if most of my comments are very surface level, still familiarizing myself with the codebase!

timhwang21 · 2018-03-15T02:05:30Z

source/ArrowKeyStepper/ArrowKeyStepper.js

-> {
+type State = ScrollIndices;
+
+class ArrowKeyStepper extends React.PureComponent<Props, State> {


NB: Question, my opinion is that this sort of type alias doesn't really do much besides obfuscate. Is the main benefit of this just to have a consistent <Props, State> signature?

timhwang21 · 2018-03-15T02:11:19Z

source/Collection/CollectionView.js

@@ -26,7 +27,7 @@ const SCROLL_POSITION_CHANGE_REASONS = {
 * Monitors changes in properties (eg. cellCount) and state (eg. scroll offsets) to determine when rendering needs to occur.
 * This component does not render any visible content itself; it defers to the specified :cellLayoutManager.
 */
-export default class CollectionView extends React.PureComponent {
+class CollectionView extends React.PureComponent {


Jw, do you have rules related to when types should be specified? ArrowKeyStepper has them, some others don't.

Some components haven't been migrated to Flow types yet. Ideally, all would have types.

timhwang21 · 2018-03-15T02:22:41Z

source/Grid/types.js

 export type CellRangeRendererParams = {
-  cellCache: Object,
+  cellCache: CellCache,


nb: Was wondering what you feel about breaking these uncontroversial changes to a separate pr so we can get them merged quicker

No objection really 😄 Focused PRs are generally good. I just did some tidying up as I went along.

timhwang21 · 2018-03-15T13:06:45Z

source/ArrowKeyStepper/ArrowKeyStepper.js

+      return {
+        scrollToColumn: nextProps.scrollToColumn,
+        scrollToRow: nextProps.scrollToRow,
+      };


This is more a comment on the new React API than anything, but I have to admit I found this hard to read, especially because _setScrollPosition() ends up calling this.setState(), _updateScrollPositionForScrollToCell() ends up maybe calling _setScrollPosition(), and this lifecycle ends up returning an object that is "magically" set to state. All 3 methods handle somewhat overlapping responsibilities yet they all do so in a different way.

(The added ending return null in _setScrollPosition() also threw me for a second, because I associated returning with calculateDerivedStateFromProps. So that method either ends with setState (which returns nothing) or returning nothing.)

I'm not sure what the best way to make this clearer would be (or if more experience would be enough). Maybe type signatures? I don't really use flow -- does it have a way to signify "this method will have a side effect via setting state" or something?

I think this comment is more pertaining to CollectionView than ArrowKeyStepper?

I think the null return from _setScrollPosition was just an oversight on my part. 😄

Ah, yeah -- comment should be there instead :)

timhwang21 · 2018-03-15T14:01:15Z

source/MultiGrid/MultiGrid.js

  }

-  _onScroll(scrollInfo) {
+  _prepareForRender() {


FWIW I'm a fan of this pattern & think it's much more readable than the large render functions that something like CollectionView has.

bvaughn · 2018-03-15T15:29:13Z

Oh, shoot. I'm sorry @timhwang21 and @TrySound for merging before addressing your comments. They didn't show up for me until after the merge. ☹️

dobryanskyy · 2018-04-03T11:00:23Z

Guys, when is this going to be merged into some npm version?

TrySound · 2018-04-03T11:06:07Z

@dobryanskyy Probably in a week.

TrySound · 2018-04-03T11:07:29Z

@dobryanskyy However it's also depends on #1053, so not sure.

/cc @wuweiweiwu

dobryanskyy · 2018-04-03T11:10:03Z

I see, thanks. You're doing a great product

wuweiweiwu · 2018-04-03T16:39:40Z

@TrySound @dobryanskyy ASAP as soon as the tests pass :) Apologies for the delay

bvaughn added 3 commits February 9, 2018 18:28

Migrated ArrowKeyStepper lifecycles

05d8943

Migrated CollectionView lifecycles

156e995

Migrated CellMeasurer examples lifecycles

4991ed4

bvaughn force-pushed the react-16-3 branch from f30a79c to 77e8bfc Compare February 10, 2018 16:01

TrySound reviewed Feb 10, 2018

View reviewed changes

bvaughn force-pushed the react-16-3 branch 2 times, most recently from 72cdf51 to a4dca4b Compare February 10, 2018 17:00

bvaughn added 4 commits February 10, 2018 10:38

Updated WindowScroller lifecycles

7a9868c

Updated Masonry lifecycles

4c4370a

Update MultiGrid lifecycles

56c3a8c

Updated Grid lifecycles (with a small TODO)

7492e8d

bvaughn force-pushed the react-16-3 branch from a4dca4b to 7492e8d Compare February 11, 2018 22:50

bvaughn added 3 commits February 11, 2018 17:28

Updated CHANGELOG

637ce70

Updated package/yarn files

c1e364b

Stashing

64b5c90

Stashing

29edb4a

Merged master

38f2ce8

Repository owner deleted a comment from codecov-io Mar 12, 2018

TrySound mentioned this pull request Mar 13, 2018

add get size helpers #1022

Merged

bvaughn added 2 commits March 14, 2018 08:22

Stashing Grid changes

a44831f

Reverted changes to Grid (to the state in master)

72d7fd1

Repository owner deleted a comment from codecov-io Mar 14, 2018

bvaughn changed the title ~~[WIP] Replace deprecated / async-unsafe lifecycles~~ Replace deprecated / async-unsafe lifecycles Mar 14, 2018

Tidying up

f9adfe6

Repository owner deleted a comment from codecov-io Mar 14, 2018

Rolled back to React 16.2 and used react-lifecycles-compat for new st…

cb39fe9

…atic lifecycle

Repository owner deleted a comment from codecov-io Mar 14, 2018

wuweiweiwu reviewed Mar 15, 2018

View reviewed changes

TrySound reviewed Mar 15, 2018

View reviewed changes

timhwang21 reviewed Mar 15, 2018

View reviewed changes

bvaughn merged commit 3c74baf into master Mar 15, 2018

bvaughn deleted the react-16-3 branch March 15, 2018 15:28

This was referenced Mar 15, 2018

Replace deprecated / async-unsafe lifecycles frontend-collective/react-sortable-tree#273

Closed

feat: Migrating Grid.js to getDerivedStateFromProps #1053

Merged

Uh oh!

Replace deprecated / async-unsafe lifecycles #1009

Replace deprecated / async-unsafe lifecycles #1009

Uh oh!

Conversation

bvaughn commented Feb 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bvaughn commented Feb 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bvaughn commented Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bvaughn commented Feb 13, 2018

Uh oh!

wuweiweiwu commented Mar 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bvaughn commented Mar 12, 2018

Uh oh!

bvaughn commented Mar 14, 2018

Uh oh!

bvaughn commented Mar 14, 2018

Uh oh!

wuweiweiwu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bvaughn commented Mar 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TrySound commented Mar 15, 2018

Uh oh!

timhwang21 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

bvaughn commented Feb 10, 2018 •

edited

Loading

bvaughn commented Feb 11, 2018 •

edited

Loading

bvaughn commented Feb 12, 2018 •

edited

Loading

wuweiweiwu commented Mar 11, 2018 •

edited

Loading

wuweiweiwu commented Apr 3, 2018 •

edited

Loading