Recompose: withPropsOnChange: when shouldMap returns false, subsequent calls have outdated "prevProps"

[grrowl]

When using withPropsOnChange with "shouldMapOrKeys" function first argument (a.k.a.shouldMap, signature (prevProps, nextProps): Boolean), prevProps is only updated when the previous call to shouldMap returned true.

I believe this is incorrect — shouldMap signals whether computedProps should be updated, and prevProps should _always_ be updated, ready for the next call to shouldMap/

Repro steps

1. On first change, shouldMap is called with prevState.prevProps
2. Because this is within getDerivedStateFromProps, if shouldMap returns false, it returns null (does not update state)
3. Therefore the next call to shouldMap, it is passed not the "current" props but the original, on-mount props.
   
   
   
   • Or, after some changes, "prevProps" will actually be "props from the last time shouldMap returned true"
   
   

I have pushed PR #667 with this change :)

[istarkov]

see PR comment

[motiazu]

+1 on this. This change broke old behavior and I heavily rely on withPropsOnChange. But now prevProps are not reliable in certain cases. And this is causing a lot of flack from the anti recompose guys here also :(

[istarkov]

I wrote in PR comment why it was done. Old behaviour looks wrong for me. Ill close this

[motiazu]

It took me a day to realize the reason my oldProps are not up to date because withPropsOnChange changed it's behavior a bit and release notes didn't mention that. Release notes should state that behavior was changed.

But I think something might have been missed here. The problem not only happens if you "calculate something based on some propsA, and don't use that propsA in decision to recalculate".
I have a case with something like this:

withPropsOnChange(
  ({ auth }, { auth: nextAuth }) => !auth.hasError && nextAuth.hasError,
  ({ auth, onExpectationFailed }) => {
    if (_.get(auth, 'error') === httpStatus.EXPECTATION_FAILED) {
      onExpectationFailed();
    }
  }
),

Because my shouldMap returns false when transitioning from hasError=true to false, the next time I have an error my old props indicate hasError=true even though they are false. So if I'm looking inside a prop's fields for my comparison, this change breaks my code.
If you would keep the oldProps in derived state, it will be consistent with previous implementation.

[istarkov]

I don't want to be consistent with previous, IMO previous implementation was wrong.
I wrote why I think so see links (PR, and link iside PR)
https://github.com/acdlite/recompose/pull/667
https://github.com/acdlite/recompose/issues/651#issuecomment-383163549

[motiazu]

I understand that, but I thought my example was different than the other examples where this breaks, as I'm relying on the same prop for calculation and for my predicate. If you think this should be different I think I need to go to a lot of old code and make sure it's not going to break now, and this should be documented as a breaking change. This could break a lot of production code based on this HOC.

[istarkov]

withPropsOnChange was never assumed to be a pattern like in your example, calling event on props change is not a good code practice. Idea was to use it as some kind of memoize function. Here in issues I wrote multiple time why this practice is bad and how to avoid. Now it become just nearer to memoize and its good that it eliminates some practises like above :-)

[motiazu]

Ok I'll search to see what you said about alternatives :)
Should still be documented as a breaking change though, regardless of what it was meant to be used for.

[istarkov]

Sorry for that,
summer in Russia is too short to spend my time documenting things ;-)

[motiazu]

@istarkov Sorry to bother but I searched to see alternatives and couldn't find something that addresses what I'm doing as a bad pattern or suggests an alternative. A link would be great :)

I'm trying to understand, did you mean the way I use shouldMap is wrong (looking inside a field of a prop) or the way I use propsMapper (not actually mapping and setting a state)?
Because this breaking change is only breaking the way I use shouldMap AFAIK.

[istarkov]

The problem that you instead of returning state, calls some side-effect function which is possibly change the parent state etc.
One of answers on similar
https://github.com/acdlite/recompose/issues/545#issuecomment-340469880

Usually it breaks idea that View is a pure function of props, now view affects world around in some not clear cases. As a usual result it causes inconsistent data on some renders, few sources of truth, parent components need to have knowledge about children internals etc, infinite loops and other fun issues.

It's somehow similar to what's written here
https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#common-bugs-when-using-derived-state

React guys did a big job to prevent such kind of activity, deprecating old lifecycle componentWillReceiveProps and providing new static getDerivedStateFromProps

To solve, usually is enough to move state outside - see controlled components. As an example
you don't need calculate this on component level if (_.get(auth, 'error') === httpStatus.EXPECTATION_FAILED) { if you use any state management like redux you could do this calculation in middleware and set error:'error message' somewhere in global state or even calling setState({error}) in fetch callback. Now all your component need is to just render Error without any checks or lifecycles

[istarkov]

The other problem in new async react that your method could be called multiple times, I dont think it will be what you expect ;-)

[motiazu]

Ok I went through a lot of reading and consulted some office mates, I think I got the idea and we changed what we did there to avoid the side effect, learned something new. But I think there is still an issue here in a different way though.

You were referring to causing side effects from propsMapper as the issue with my code, which is also true, but you didn't respond to why shouldMap looking at inner fields should have been broken.
The way it's currently built (not saving oldProps if shouldMap returns false) breaks the option for shouldMap to be a custom function. Only the string array option is viable now, otherwise you could have unexpected results (as I have gotten).

[istarkov]

Internally its just call to pick and shallowEqual and this has no difference if you call any other function. So please provide an example where you think that string array option is viable and any other function is not. And I respond few times why it have current behaviour - memoize is keyword.

[motiazu]

Yes but memoize is for the propsMapper rather than the shouldMap isn't it?
The example I had up there would not work because it uses the function. I get that the default implementation for the string array is just a pick, but if you allow using a function you give the developer a lot of other options. Like this one:

({ auth }, { auth: nextAuth }) => !auth.hasError && nextAuth.hasError,
So in my case, I'm trying to catch a tranisition from false to true on hasError.
I'll break down what happens with each change on the previous implementation and the new one, so the difference will be clear:

Old implementation

| Actual value | prevProps | nextProps | comment |
-------------|:-----------:|---------|:--------|
| false | false | false |
| true | false | true |
| false | true | false |
| true | false | true |

New implementation

| Actual value | prevProps | nextProps | comment |
-------------|:-----------:|---------|:--------|
| false | false | false |
| true | false | true |
| false | true | false |
| true | true | true | prevProps has true because we don't save the previous values in the change before |

Notice how the prevProps is always correctly stating the last value sent to nextProps except for the last row in the new implementation.

[istarkov]

Good catch,
let me think.
The problem I think with all that comparisons will arise when react become async, so it will be possible that some updates will be replayed multiple times, memoize will work, comparisons will not. BTW to support your case seems like for now will be better to revert. Tests and PR are welcome.

[istarkov]

In the future code above will be a problem, as you could get multiple heavy recalc during render phase

[motiazu]

Do you think reverting to componentWillRecieveProps will be different in number of comparisons than returning { prevProps: nextProps } in getDerivedState? I didn't do my research on async rendering yet so maybe that's what I'm missing here.
I'd love to send a PR and some test cases for this

[istarkov]

Returning will be no different