Skip to content
This repository has been archived by the owner on Sep 10, 2022. It is now read-only.

withState state updater callback function does not return value #466

Closed
plakak opened this issue Aug 7, 2017 · 20 comments
Closed

withState state updater callback function does not return value #466

plakak opened this issue Aug 7, 2017 · 20 comments
Assignees

Comments

@plakak
Copy link

plakak commented Aug 7, 2017

Maybe I'm not doing it right, but based on the documentation I am able to provide a callback function to state updater that is executed after component re-render.

Both forms accept an optional second parameter, a callback function that will be executed once setState() is completed and the component is re-rendered.

I would assume that I will get the new state value (just like with setState's callback). Or that props would be already changed to reflect new state since it's called after re-render.

I can't make it work; here's a JS bin with the issue: https://jsbin.com/pobonu/edit?js,console,output
Is my implementation/assumption wrong or something's wrong with recompose?

@plakak plakak changed the title withState state updater callback function is empty withState state updater callback function does not return value Aug 7, 2017
@Markionium
Copy link

As far as i know the setState callback does not return you the new state?

It seems to me as if it is working correctly. The props do not reflect the changed value because the props object that you have available in the callback is the "old" props object. As the handler was created with the old props object.

See this edited JS bin https://jsbin.com/nizigijipu/edit?js,console,output

@plakak
Copy link
Author

plakak commented Aug 11, 2017

Ok, you're right, setState callback does not return a new function. But after it gets fired console.log(this.state) shows the new updated state. I think this should be the same for withState.

Because right now I can't find any use for a callback function - whats the use of it at this point if I don't have updated value when it fires? I need to do something with the new state after it gets updated and it seems that I can't do it with withState at the moment.

@Markionium
Copy link

I kind of agree that the callback function might not be very useful in the case that you want to use the new state. This sounds more like a case where you'd benefit from using lifecycle (https://github.com/acdlite/recompose/blob/master/docs/API.md#lifecycle) That seems more in line with the React documentation.

As the React docs mention:

The second parameter to setState() is an optional callback function that will be executed once setState is completed and the component is re-rendered. Generally we recommend using componentDidUpdate() for such logic instead.

@plakak
Copy link
Author

plakak commented Aug 11, 2017

Thanks for explenation. This is a valid point, yet I would prefer not to include another HOC like lifecycle. At this point I could just make a class with normal state & lifecycle. Would be great if callback could return a new state :).

@wuct
Copy link
Contributor

wuct commented Aug 14, 2017

@plakak in your case, you can pass the new state to the handler function as a argument, or compute the new state and assign it to a local variable inside the handler function.

@plakak
Copy link
Author

plakak commented Aug 16, 2017

@wuct yes, this is how I got around this - by local variable. But callback would be better in my opinion for such async change. Right now I don't really see how it could be useful if I'm not sure my state got updated when it's called.

@wuct
Copy link
Contributor

wuct commented Aug 16, 2017

Dose React pass the new state to setState second callback?

@plakak
Copy link
Author

plakak commented Aug 16, 2017

No, it does not. But the callback is executed after the new state is set & component re-rendered so doing this.state.foo in this callback returns the new state.

@wuct
Copy link
Contributor

wuct commented Aug 16, 2017

@plakak I got your point and I agree with you. It is better to pass the new state in our case. Would you like to create a PR for it?

@plakak
Copy link
Author

plakak commented Aug 17, 2017

@wuct Sure, I'll look into it this weekend.

@istarkov
Copy link
Contributor

Just question from my side.
What you want to do with that state in callback? Any real example.
As Im not sure we can accept change the only reason for it is to provide some sideeefect.

@plakak
Copy link
Author

plakak commented Aug 22, 2017

@istarkov Sorry for a late reply - I've been busy lately.

In my project, I have few components, each of them have isActive state (boolean). It's in a global Redux store. One of those components has additional internal state of filters which is an array of strings. If there is at least one filter & component is inactive I should activate the global component state, if the last filter is removed I should deactivate it.

For me, it would be ideal that after filters state gets updated I can check if I have some filters or not and update my global state.

Yes, of course, I can add lifecycle method to listen on every change but the reason why I use recompose is to minimize the amount of code and overhead. If I need state and lifecycle method from recompose, just to check state change, I can write a class instead. In my opinion, if I have a HOC that provides me with a state I should be able to get that new updated state there and not to rely on other HOCs just to support this one. But that's maybe just me :)

@wuct
Copy link
Contributor

wuct commented Aug 23, 2017

So there is many isActives in the redux store and a filters in a local state, and the filters will affect isActives. If it's the case, probably lifting filters into the redux store and computing all states in reducers is a better approach.

@plakak
Copy link
Author

plakak commented Aug 23, 2017

@wuct If two or more components would benefit from using a given state then I put it into Redux. My other components won't and shouldn't care about filters state of this component. It's local and I want to leave it local. Only the change from inactive to active is something that affects other components and only this change I want to propagate to Redux. Making a special case in reducer that only one of my 20 components would use doesn't seem right to me.

The outcome is still the same for my app; something is active or not.
Redux shouldn't care if I do dispatch(setActive(category)) or if (filters.length) dispatch(setActive(category)) in my component.

@wuct wuct self-assigned this Aug 24, 2017
@istarkov
Copy link
Contributor

@plakak Sometimes you need redux not only to share state between components,
I wrote about here #368 (comment)
The idea that to make view as pure as possible you need to lift state upper in a tree, or use redux which is fine for such things.

@istarkov
Copy link
Contributor

istarkov commented Aug 26, 2017

PS: For now I hosts some reducers in the same folder as container, like in popular react boilerplate here https://github.com/react-boilerplate/react-boilerplate/tree/master/app/containers/HomePage
That idea allows me to use redux more frequently, and seems like it was good idea. As having a pure views really allowed us to reduce amount of strange errors in production.

And having that Container specialized reducer is hosted with Container itself, it make me feel that it's not some kind of global state, but the state needed for this Container even it isn't shareable.

@istarkov
Copy link
Contributor

In my mind I always think about any Component which has internal state will it work or not if it will be placed inside some kind of <Debounce> component, so will be rendered not every parent component update. (I've played a lot with delaying Components rendering based on visibility etc and it is somehow similar with Debounce) In most cases if component state is dependent on props it's impossible to make this component to work with Debounce idea. So any visual smart optimisations can break the behaviour of my program. So I try to avoid such state.

@plakak
Copy link
Author

plakak commented Aug 28, 2017

@istarkov Maybe I'm misunderstanding your point, but my component doesn't depend on external props like in the example(#368) outerCounter. I'm not passing any props to this component. It only uses his own state of filters & isActive flag from Redux via connect. Filters are an internal state that is not changed by anything from the outside - just user interaction with the component. If you put my component in <Debounce> it should work fine.

But even if you have something different in mind and I would put filters in redux I don't really see what harm would do being able to know new state after the update. I've worked around this in my case and won't insist if you think that's not how this callback should work. For me having empty callback is a bit weird (just like with React's setState) as I'm used to using them to get results of async operations.

@istarkov
Copy link
Contributor

To be clear after reading your comments what I understand:
You want to change some global state as a reaction on an internal state change.
Something like:
onClick = {() => this.setState(() => newState, () => ChangeGlobalState() )}
Is it?

@istarkov
Copy link
Contributor

istarkov commented Aug 28, 2017

If yes now about harm:
We have
this.setState(() => newState, () => ChangeGlobalState() )
This means

  1. User call handle
  2. set newState
  3. call component render
  4. ChangeGlobalState according to your change

So at least for one render global state could be inconsistent with local state. May be in your case you don't use global state at all etc no problem. But in general it can be a problem.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants