ReSub v2 setState not working? #131
Comments
|
Any chance that |
|
Just whipped up an example with 2.0.0-rc.1 and things work as expected - here's a snippet |
|
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: 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 I get this: 🤔 I am usually doing something wrong. But I can't see it. And it works in this style with resub@1.2.2 |
|
(although my log messages are already suspicious there) |
|
I see what's going on here - |
|
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;
} |
This will handle the case where _buildState relies on a value that can be changed by calling setState Fixes microsoft#131
This will handle the case where _buildState relies on a value that can be changed by calling setState Fixes #131
|
Didn't mean for this to auto-close - publishing rc.2, lemme know if that helps you |
|
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:
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. |
|
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 Lines 77 to 79 in 4fbf16b So that was a breaking change for me. Easy to fix, but if there is a migration guide it might be an entry |
|
Hey, Secondly, with ReSub you should initialize your state inside the _buildState method, if 'initialBuild' is true. That is beacuse of ReSub setting some internal values to the State in the constructor of ComponentBase. And yes, that could be stated in the migration guide, but in my opinion, the documentation stated that already clearly before: |
|
@BiggA94 thanks for that link on setState - I'm still getting up to speed on all things React so that is useful reading. No disagreement that I should have used initialBuild - I conveniently side-stepped the "debate" (knowing it was the wrong style) in my comment, I just wanted to point out that it was a breaking behavior change in the context of a possible migration guide. The goal would be to reduce future support load for maintainers - any breaking change inevitably turns in to 2-3 issues in my experience if not documented explicitly as such |
|
@mikehardy The more I think about your example, the more I think, the usage of buildState is not correct here. You don't need to return the whole state in the _buildState, only the objects that should be updated. Also I think I found a 'Problem' with the concept of v2.. @berickson1 |
This was critical to untangling my work project, thank you.
This is where I was tangled. I found I was not even able to rely on the new state argument being updated after setState. But it was true (in my project) that there was separation between the items in state I was managing in the component with UI handlers and the items I that were in an external Store. So by returning Partial State objects from _buildState that were only concerned with the external Store, everything is working in v1's method/render lifecycle, _buildState had access to an updated / correct state so it wasn't a concern. |
|
@mikehardy I think I have to dig deeper into the react lifecycle to find the correct solution to this.. Could you provide a repro, or show the code, where you see, that you couldn't rely on the new state argument? |
|
Not quickly, but I think the heart of a minimal repro will be do have the setState call in the handler be asynchronous. The setup on this screen is with react-navigation It may be possible to mock this up with timers etc (that's how I'd do it) but unfortunately I'm out of time to play with it for a while |
|
Just to clarify a little bit on expected internal behaviour or ReSub
|
|
As for Mike's last question - I tried doing a setTimeout before calling setState and couldn't repro :( |
|
echoing that, my main takeaway is that I was dealing with state terribly (in my subjective opinion), so in that way the v1->v2 behavior change has been quite illuminating and resulted in a big project improvement. Also I took a quick stab at a reproduction starting with the very code I saw it in and I must retract my assertion that the new 3rd argument was not updated. I can no longer reproduce it. I assume it was still a reference to this.state. vs state.. So I return to my previous assertion that with 2.0.0-rc.2 (and my state-handling reorganization) my project is working fine. |
|
As part of this v2 moveover, we're continuing to try to simplify the happy path, and make it harder to hose yourself. So it's still really good for us to flesh out cases like this, and see how we can add more exceptions/checking to help people down the happy path. |
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.
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 testexecutionThe text was updated successfully, but these errors were encountered: