ReSub v2 setState not working?

[mikehardy]

Hey you all - I'm testing out the 2.0.0-rc.1 and vs 1.2.2 I have a component that fails to setState correctly.

  private _onNextPage = () => {
    console.log(
      'TIPI::_onNextPage - was on page ' + this.state.pageIndex + ' (max is ' + this.MAX_PAGE + ')'
    );
   let nextIndex = this.state.pageIndex + 1;
    this.setState({ pageIndex: nextIndex >= this.MAX_PAGE ? this.MAX_PAGE : nextIndex });
    console.log('TIPI::_onNextPage - will be on page ' + this.state.pageIndex);
  };

It's a simple pager, and I get 'will be on page 0' (the initial state) every click with 2.0.0-rc.1 but it increments nicely with 1.2.2.

That's a vague report in and of itself but it is a clear regression with this code between those two versions.

I'm trying to get a repro together but in the meantime I also see this on an npm test execution

mike@isabela:~/work/react-random/resub (master) % npm test

> resub@2.0.0-rc.1 test /home/mike/work/react-random/resub
> run-s clean:* karma:single-run


> resub@2.0.0-rc.1 clean:dist /home/mike/work/react-random/resub
> rimraf dist*


> resub@2.0.0-rc.1 karma:single-run /home/mike/work/react-random/resub
> karma start --singleRun

Starting type checking service...
Using 1 worker with 2048MB memory limit
Type checking in progress...
ℹ ｢wdm｣: Compiled successfully.
ℹ ｢wdm｣: Compiling...
ERROR in /home/mike/work/react-random/resub/test/AutoSubscribe.spec.tsx(128,7):
TS2417: Class static side 'typeof OverriddenComponent' incorrectly extends base class static side 'typeof SimpleComponent'.
  Types of property 'getDerivedStateFromProps' are incompatible.
    Type 'GetDerivedStateFromProps<SimpleProps, SimpleState>' is not assignable to type 'GetDerivedStateFromProps<unknown, unknown>'.
      Types of parameters 'nextProps' and 'nextProps' are incompatible.
        Property 'ids' is missing in type 'Readonly<unknown>' but required in type 'Readonly<SimpleProps>'.
Version: typescript 3.5.3
Time: 2315ms
ℹ ｢wdm｣: Compiled successfully.

[berickson1]

Any chance that pageIndex is set inside _buildState?

[berickson1]

Just whipped up an example with 2.0.0-rc.1 and things work as expected - here's a snippet
If you could provide any more context, that would be helpful to investigate

[mikehardy]

Here's a reproduction - I didn't mean to leave it vague but you are faster to respond than I can code tiny repro apps :-)

Do this:

npx react-native init ReSubStateTest
cd ReSubStateTest
npm add resub@2.0.0-rc.1

Replace your entire App.js with this:

import React from 'react';
import { View, Button, Text } from 'react-native';
import { ComponentBase } from 'resub';

interface CounterProps {}

interface CounterState {
  counter: number;
}

class Counter extends ComponentBase<CounterProps, CounterState> {

  _buildState(props, state) {
    if (!this.state || this.state.counter == undefined) {
      console.log('_buildState - no initial state, initializing to 0');
      return { counter: 0 };
    } else {
      console.log('_buildState - counter already set to ' + this.state.counter);
      return { counter: this.state.counter };
    }
  }

  _handleClick = () => {
    console.log('_handleClick - incrementing from ' + this.state.counter + ' to ' + (this.state.counter + 1));
    this.setState({ counter: this.state.counter + 1 });
    console.log('_handleClick - counter is now ' + this.state.counter);
  }

  render() {
    return (
      <View>
        <Text>Counter is { this.state.counter }</Text>
        <Button title="Increment" onPress={this._handleClick}/>
      </View>
    );
  }
}

const App: () => React$Node = () => {
  return (
    <>
      <Counter/>
    </>
  );
};

export default App;

Run it? (start your android emulator and npm start + npm run android or the equivalent for ios)

I get this:

 LOG  _buildState - no initial state, initializing to 0
 LOG  _handleClick - incrementing from 0 to 1
 LOG  _handleClick - counter is now 0
 LOG  _buildState - counter already set to 0
 LOG  _handleClick - incrementing from 0 to 1
 LOG  _handleClick - counter is now 0
 LOG  _buildState - counter already set to 0
 LOG  _handleClick - incrementing from 0 to 1
 LOG  _handleClick - counter is now 0
 LOG  _buildState - counter already set to 0
 LOG  _handleClick - incrementing from 0 to 1
 LOG  _handleClick - counter is now 0
 LOG  _buildState - counter already set to 0

🤔 I am usually doing something wrong. But I can't see it. And it works in this style with resub@1.2.2

 LOG  _buildState - no initial state, initializing to 0
 WARN  Warning: componentWillMount has been renamed, and is not recommended for use. See https://fb.me/react-async-component-lifecycle-hooks for details.

* Move code with side effects to componentDidMount, and set initial state in the constructor.
* Rename componentWillMount to UNSAFE_componentWillMount to suppress this warning in non-strict mode. In React 17.x, only the UNSAFE_ name will work. To rename all deprecated lifecycles to their new names, you can run `npx react-codemod rename-unsafe-lifecycles` in your project source folder.

Please update the following components: Counter
 WARN  Warning: componentWillReceiveProps has been renamed, and is not recommended for use. See https://fb.me/react-async-component-lifecycle-hooks for details.

* Move data fetching code or side effects to componentDidUpdate.
* If you're updating state whenever props change, refactor your code to use memoization techniques or move it to static getDerivedStateFromProps. Learn more at: https://fb.me/react-derived-state
* Rename componentWillReceiveProps to UNSAFE_componentWillReceiveProps to suppress this warning in non-strict mode. In React 17.x, only the UNSAFE_ name will work. To rename all deprecated lifecycles to their new names, you can run `npx react-codemod rename-unsafe-lifecycles` in your project source folder.

Please update the following components: Counter
 WARN  Warning: componentWillUpdate has been renamed, and is not recommended for use. See https://fb.me/react-async-component-lifecycle-hooks for details.

* Move data fetching code or side effects to componentDidUpdate.
* Rename componentWillUpdate to UNSAFE_componentWillUpdate to suppress this warning in non-strict mode. In React 17.x, only the UNSAFE_ name will work. To rename all deprecated lifecycles to their new names, you can run `npx react-codemod rename-unsafe-lifecycles` in your project source folder.

Please update the following components: Counter
 LOG  _handleClick - incrementing from 0 to 1
 LOG  _handleClick - counter is now 0
 LOG  _handleClick - incrementing from 1 to 2
 LOG  _handleClick - counter is now 1
 LOG  _handleClick - incrementing from 2 to 3
 LOG  _handleClick - counter is now 2

[mikehardy]

(although my log messages are already suspicious there)
It's late here so I'll be AFK till the morning, but I appreciate the look already.
Hopefully it's something obvious, I was enjoying getting rid of the lifecycle method deprecations, and was hoping I'd test the RC for you without stumbling on anything :-)

[berickson1]

I see what's going on here - setState triggers a getDerivedStateFromProps, which runs _buildState. I think _buildState is going to need to have prevState passed to it - this.state probably can't be trusted in _buildState

[berickson1]

The short-term mitigation is to remove setting current state back to state - but that's not a true "fix" - I'm investigating now but I think it'll involve passing state into buildState and having to use that

  _buildState(props, state) {
    if (!this.state || this.state.counter == undefined) {
      console.log('_buildState - no initial state, initializing to 0');
      return { counter: 0 };
    }
  return undefined;
  }

[berickson1]

Didn't mean for this to auto-close - publishing rc.2, lemme know if that helps you

[mikehardy]

Good morning :-), I actually didn't see you'd published rc.2 until after I tested but I just added berickson1/resub#incomingState and built it in node_modules to test - same effect.

It does work with this code now as _buildState:

  _buildState(props, initial, state) {
    if (!state || state.counter == undefined) {
      console.log('_buildState - no initial state, initializing to 0');
      return { counter: 0 };
    } else {
      console.log('_buildState - counter already set to ' + state.counter);
      return { counter: state.counter };
    }
  }

👏 👏

(basically doing what the new README says - adding a state variable as a third variable, and referencing that local var instead of this.state)

I'm still a little confused on why referencing state immediately after setting it returns a stale value. That is unexpected and I may have just never noticed it before, but it is not a regression from v1->v2 it was there in v1

So I have three side notes for this issue:

• npm test produces an ERROR but does not fail?
• the README might get a migration guide linked around from other places with a fuller description
• using state immediately after setting it is odd? [update: setState is mildly async, this is not a resub issue]

But clearly the actual issue is resolved, which is great. That was my only issue with rc.1 so I believe I can rely on it in my project. I'm not using it in a very complicated way, but it is fundamental to the project, so hopefully that helps as a test.

As a final side note I really appreciate how the team ships source ready to build with reactxp releases so that (if needed) you can use something like patch-package to patch the .ts files then as postinstall cd into node_modules && install && build. I may not have started contributing personally if that wasn't the case. I liked the style so much I copied it and made it a requirement when we migrated react-native-device-info to typescript - I made sure people had the source and a working build setup in their normal project install so I could capture what I think of as "drive by contributions" from library users (which usually start as node_modules hacks I think). I notice that resub doesn't do this but it seems close, it's just missing the .eslintrc and a couple other things and it would build.

[mikehardy]

Sorry, I'm full of side-notes. I just remembered that a separate breaking change was that I had a pattern (valid or not) in my constructors where I would:

this.state = {
  somevar: "initial state",
};

...initializing to the empty state during construction. Is that useful or is it an anti-pattern? Side-stepping that debate, it was not an error in v1, but in v2 it triggered

ReSub/src/ComponentBase.ts

Lines 77 to 79 in 4fbf16b

	if (!internalState._resubGetInstance) {
	throw new Error('Resub internal state missing - ensure you aren\'t setting state directly in component construtor');
	}

So that was a breaking change for me. Easy to fix, but if there is a migration guide it might be an entry