Remove usages of async-unsafe lifecycle methods #919

Hypnosphi · 2018-04-03T01:01:29Z

Fixes #897

Hypnosphi · 2018-04-03T01:02:59Z

test/components/connect.spec.js

@@ -2121,7 +2120,7 @@ describe('React', () => {

      @connect(null)
      class Parent extends React.Component {
-        componentWillMount() {
+        UNSAFE_componentWillMount() {


When I tried to change it to componentDidMount it broke the test. Maybe it should be changed to constructor, I'm not sure

What error did you get with cDM? The cWM function is not consequential here anyways; this is testing for unmount cleanups. So, I'd be fine with removing it and putting a dispatch outside of the ReactDOM.render.

data was null

timdorr · 2018-04-03T17:24:00Z

src/components/connectAdvanced.js

-      }
-
-      shouldComponentUpdate() {
+      shouldComponentUpdate(nextProps) {


This feels like a hack with bad side effects. Can we use getSnapshotBeforeUpdate instead?

Will try that. Is it called before sCU?

Looks like it's not: https://reactjs.org/docs/react-component.html#updating

I can try to move selector from field on instance to field on state, then it will be available in static getDerivedStateFromProps

timdorr · 2018-04-03T17:44:03Z

src/components/connectAdvanced.js

@@ -248,6 +247,10 @@ export default function connectAdvanced(

      render() {
        const selector = this.selector
+
+        // Handle forceUpdate
+        if (!selector.shouldComponentUpdate) selector.run(this.props)


Shouldn't this be handled by sCU above?

sCU doesn't run when forceUpdating

gaearon · 2018-04-03T18:07:39Z

Btw let us know when you'll need a final review for whether it's async-safe.
I'm not actively watching right now but we could take a look when PR is ready.

Hypnosphi · 2018-04-04T23:59:19Z

@gaearon I think it's ready for review

pke · 2018-04-05T14:35:14Z

test/components/connect.spec.js

@@ -2009,7 +2008,7 @@ describe('React', () => {

      @connect(null, mapDispatchFactory, mergeParentDispatch)
      class Passthrough extends Component {
-        componentWillUpdate() {
+        componentDIdUpdate() {


There is a typo here? componentDIdUpdate

Will fix & add test

Just commited a fix for you.

pke · 2018-04-05T14:36:13Z

package.json

@@ -53,7 +53,8 @@
    "lodash": "^4.17.5",
    "lodash-es": "^4.17.5",
    "loose-envify": "^1.1.0",
-    "prop-types": "^15.6.1"
+    "prop-types": "^15.6.1",
+    "react-lifecycles-compat": "^1.1.4"


Why do you need the polyfill when you changed all the deprecated functions?

He's using getDerivedStateFromProps.

I am using that too without a polyfill. No more warnings (in my own code).

It's for use with older versions of React. It's not to deal with deprecation warnings, it's to polyfill those lifecycle methods for React <= 16.2.

Ah right, didn't think it that way around.
That brings me to a question for my own lib. What if I want to support the new and old Context API I guess I could just check if React.createContext is a function and then use it if avail?

@pke : there's a distinct difference in how you declare and use the old and new context APIs. See my PR in #898 for an example.

We may need to do that check for React.createContext so we can use a polyfill like create-react-context, but the old and new context APIs aren't interchangeable.

New context API is probably going to require breaking changes to React Redux's public API for supporting multiple stores, making it a major version bump.

markerikson · 2018-04-05T20:48:40Z

src/components/connectAdvanced.js

@@ -129,7 +138,9 @@ export default function connectAdvanced(
          `or explicitly pass "${storeKey}" as a prop to "${displayName}".`
        )

-        this.initSelector()
+        this.state = {
+          selector: this.createSelector()


I haven't looked at what StrictMode warns about yet. Does it complain about instance variables? Is that why we're putting the selector into state?

No it doesn't.

This is because getDerivedStateFromProps is static, so one can't access instances from it

Ahhh, I see it now. Got it.

gaearon · 2018-04-06T00:08:26Z

src/components/connectAdvanced.js

@@ -86,6 +91,11 @@ export default function connectAdvanced(
    [subscriptionKey]: subscriptionShape,
  }

+  function getDerivedStateFromProps(nextProps, prevState) {
+    prevState.selector.run(nextProps)


This looks mutative. What does it actually do?

I'm worried that this won't be safe and then avoiding the warning just gives a false sense of security.

The method signature is intentionally "get" to indicate it should be pure.

It's not pure but it's idempotent: when you run in twice, it has the same effect as if it was just once.

I can try to make it pure by moving all the fields from selector to state though.

Let's try that!

Did that. It required some minor changes in tests: a7ba298#diff-b5fd7a42ea1b8efa382416b4a323003c

It's quite clear why setState calls count changed, but as for mapStateToPropsCalls, I'm not so confident

timdorr · 2018-04-06T02:18:04Z

Well, now that the changes are becoming more significant, I wonder if we should pick #856 back up.

markerikson · 2018-04-06T03:07:35Z

I think my changes in #898 supersede #856 .

We might want to consider how these would fit together.

peacechen · 2018-04-08T22:40:50Z

I'm testing with this fork and components subscribed to redux state using mapStateToProps don't trigger componentDidUpdate on state changes. Is there something else I need to set up?

Hypnosphi · 2018-04-09T13:46:12Z

@peacechen it works OK on my project. Can you share some parts of your setup (maybe as a reproduction repo), so that I could debug it?

peacechen · 2018-04-09T14:35:19Z

It's a pretty large app. I'll try to create a small sample project to recreate the behavior.

timdorr

I'll pull this into some apps to see what breaks. But I'm not seeing any major concerns with the changes proposed.

I would be interested in combining with #856 to move to a full React 16 dep. We still need to do 16.0-16.2 support, so the polyfill is necessary. But I don't consider that a huge issue for the moment.

markerikson · 2018-04-20T22:25:47Z

Yeah, I suspect there's a lot of overlap between the two. I assume this would probably merit a v6.0 major version? So, notionally v6 would be "compat with React 16.3+", and v7 would be "super-duper React async rendering awesomeness" .

timdorr · 2018-04-21T22:21:07Z

6.0 would be 16+. This supports <16.2, and Jim's PR does 16+. So, 16.0-16.2 would be covered. Definitely a major, though.

timdorr · 2018-05-14T02:01:54Z

Anyone else want to second this? I can push a 5.1.0-beta.1 to give it a whirl elsewhere.

ematipico · 2018-06-20T13:08:38Z

I could try to integrate it into a small project of mine and leave some feedback, once the beta is out on npm.

timdorr · 2018-06-21T16:33:24Z

OK, I'm going to merge this in and push a 5.1.0-test.1 release. Please give this a whirl! We can look into some of the BC-breaking stuff next.

timdorr · 2018-06-21T18:32:43Z

Found a big bug with the gDSFP change in 16.4: https://reactjs.org/blog/2018/05/23/react-v-16-4.html#bugfix-for-getderivedstatefromprops

Because it's getting called for any render, it's getting double-called for most updates. That leads to it setting the sCU flag to true and then false on the 2nd call.

I've got to do a deeper dive, but I don't have time at the moment.

timdorr · 2018-06-21T20:18:14Z

Well, I think (hope) it's just an issue with our test suite. So, react-redux@next is now on npm to try out.

* Reformat to make VS Code less annoying * Failing test in <StrictMode> * Remove usages of async-unsafe lifecycle methods * Remove usage of UNSAFE_componentWillMount in test * Move `selector` to state in order to be able to use gDSFP * codeDIdCommit * Stop using mutation * Upgrade react-lifecycles-compat

timdorr and others added 3 commits April 2, 2018 12:58

Reformat to make VS Code less annoying

7b4e162

Failing test in <StrictMode>

6ad378b

Remove usages of async-unsafe lifecycle methods

20de413

Hypnosphi commented Apr 3, 2018

View reviewed changes

timdorr reviewed Apr 3, 2018

View reviewed changes

Hypnosphi added 2 commits April 4, 2018 02:42

Remove usage of UNSAFE_componentWillMount in test

94f0b8a

Move selector to state in order to be able to use gDSFP

9ee1a36

pke reviewed Apr 5, 2018

View reviewed changes

codeDIdCommit

7344f3d

markerikson reviewed Apr 5, 2018

View reviewed changes

gaearon reviewed Apr 6, 2018

View reviewed changes

Stop using mutation

a7ba298

markerikson mentioned this pull request Apr 9, 2018

Connected components cannot use new 16.3 lifecycles #915

Closed

Upgrade react-lifecycles-compat

27b2d4b

timdorr approved these changes Apr 20, 2018

View reviewed changes

Hypnosphi mentioned this pull request May 10, 2018

Fix <StrictMode> warnings #897

Closed

markerikson mentioned this pull request May 13, 2018

Consolidated React 16.3+ compatibility and roadmap discussion #950

Closed

LPGhatguy mentioned this pull request May 15, 2018

Rewrite connect2 to be a lot simpler Roblox/roact-rodux#11

Merged

Merge branch 'master' into strict-mode

48c8a75

timdorr merged commit d16f759 into reduxjs:master Jun 21, 2018

markerikson mentioned this pull request Jul 18, 2018

[5.1.0-test.1] Connected components don't re render after state update #965

Closed

timdorr mentioned this pull request Aug 13, 2018

Convert tests to react-testing-library, remove enzyme and test-renderer #996

Closed

snyk-bot mentioned this pull request Aug 14, 2019

[Snyk] Upgrade react-redux from 5.0.6 to 5.1.1 Watemlifts/wagtail#48

Open

This was referenced Aug 30, 2019

[Snyk] Upgrade react-redux from 5.0.7 to 5.1.1 iliutastoica/react-slingshot#3

Open

[Snyk] Upgrade react-redux from 5.0.7 to 5.1.1 jakeherp/react-slack#6

Merged

[Snyk] Upgrade react-redux from 5.0.7 to 5.1.1 iliutastoica/robofriends-pwa#4

Open

This was referenced Sep 22, 2019

[Snyk] Upgrade react-redux from 5.0.6 to 5.1.1 Matthelonianxl/lisk-nano#9

Open

[Snyk] Upgrade react-redux from 5.0.6 to 5.1.1 Matthelonianxl/lisk-hub#9

Open

[Snyk] Upgrade react-redux from 5.0.7 to 5.1.1 maxiplux/ReduxSimpleStarter#3

Merged

dependabot bot mentioned this pull request Mar 18, 2021

chore(deps): bump react-redux from 4.4.10 to 5.1.2 hkosova/swagger-ui#2

Closed

Remove usages of async-unsafe lifecycle methods #919

Remove usages of async-unsafe lifecycle methods #919

Conversation

Hypnosphi commented Apr 3, 2018

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

Hypnosphi Apr 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Apr 3, 2018

Hypnosphi commented Apr 4, 2018

pke Apr 5, 2018 • 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

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

markerikson Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hypnosphi Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon Apr 6, 2018 • edited Loading

Choose a reason for hiding this comment

Hypnosphi Apr 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timdorr commented Apr 6, 2018

markerikson commented Apr 6, 2018

peacechen commented Apr 8, 2018

Hypnosphi commented Apr 9, 2018

peacechen commented Apr 9, 2018

timdorr left a comment

Choose a reason for hiding this comment

markerikson commented Apr 20, 2018 • edited Loading

timdorr commented Apr 21, 2018

timdorr commented May 14, 2018

ematipico commented Jun 20, 2018

timdorr commented Jun 21, 2018

timdorr commented Jun 21, 2018

timdorr commented Jun 21, 2018

Hypnosphi Apr 3, 2018 •

edited

Loading

pke Apr 5, 2018 •

edited

Loading

markerikson Apr 5, 2018 •

edited

Loading

Hypnosphi Apr 5, 2018 •

edited

Loading

gaearon Apr 6, 2018 •

edited

Loading

Hypnosphi Apr 6, 2018 •

edited

Loading

markerikson commented Apr 20, 2018 •

edited

Loading