Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make setState return a promise #2642

Closed
ghost opened this issue Dec 2, 2014 · 49 comments
Closed

Make setState return a promise #2642

ghost opened this issue Dec 2, 2014 · 49 comments

Comments

@ghost
Copy link

ghost commented Dec 2, 2014

setState() currently accepts an optional second argument for callback and returns undefined.

This results in a callback hell for a very stateful component. Having it return a promise would make it much more managable.

It is somewhat of a convention in JS world to have API symmetric - if a method accepts a callback, it returns a promise. if it returns a promise, it can accept a callback.

In my case, I have an editable table. If user presses enter, he is moved to the next row (call setState). If the user is on a last row, I'd like to create a new row (call setState) and focus on this new row (call setState). Currently the only way is to achieve this is to have a nested callback hell.

@sebmarkbage
Copy link
Collaborator

Do you have any ideas about what to do with React.render which also accepts a callback but also returns a value?

There are a few other techniques that can be used. You can have the row's componentDidMount call focus so that it's auto-focused when it is created.

The new style refs are callbacks that get fired when the child is mounted. That can be used to trigger a focus without the need of wrapping it.

You can also use the componentDidUpdate life-cycle hook as a component gets updated to ensure that the focused thing (according to your business logic) always retains focus (e.g. the newest one).

I think that these are probably better alternatives that don't rely on the imperative nature of the source of your state change. E.g. you're tying it to the action that triggered the event. What if you add another state transition to the same state? Isn't it confusing that you can end up in the same state with different side-effects? That's part of the beauty of React, that you can avoid that.

Do you think that one of those patterns could replace your callback?

Even if it isn't... Honestly, the current batching strategy comes with a set of problems right now. I'm hesitant to expand on it's API before we're sure that we're going to keep the current model. I think of it as a temporary escape until we figure out something better.

Does that seem fair?

@gaearon
Copy link
Collaborator

gaearon commented Dec 11, 2014

In my experience, whenever I'm tempted to use setState callback, I can achieve the same by overriding componentDidUpdate (and/or componentDidMount).

@karlguillotte
Copy link

I was wondering the same. Thanks @gaearon for the trick it works as expected.

@lutherism
Copy link

Using componentDidUpdate aside, Promises are a logical way of handling the post setState hook. And promises reflect the current state-of-opinion in the javascript community.

this.setState({
  selected: input
}).then(function() {
  this.props.didSelect(this.state.selected);
}.bind(this));

Is common React code, and much more readable than a optional callback argument.

Bump.

@sebmarkbage
Copy link
Collaborator

Why would you use that pattern over this?

this.setState({
  selected: input
});
this.props.didSelect(input);

It is in fact probably an anti-pattern to fire an event after updates in that case because it may triggers multiple rendering passes and multiple layout passes which has significantly worse performance implications.

@lutherism
Copy link

The two code examples are functionally different. Anytime the parent need the child's updated DOM in place, or pending states resolved would be a candidate for the promise.

To come up with a contrived example, lets say the selection state changes the rendered component's width, and the parent needs to calculate a new position to account for this new child width.

Yes these DOM issues can be accounted for with componentDidUpdate, or others. But that's not the point. The point of this issue is to transition the setState interface to something more inline with JS community standards.

@jwarkentin
Copy link

The problem with existing JS community practices is that they can often make code difficult to reason about. The nice thing about lifecycle methods is that they make it easy to reason about. There aren't side effects happening in other unexpected places due to some callback. If everything can be handled through lifecycle methods then I would personally advocate removing the setState() callback altogether.

@zzz6519003
Copy link
Contributor

what's a setState callback XD @gaearon

@jwarkentin
Copy link

@zzz6519003 It's a callback that can be passed as the second argument to setState() to be called when it is done. See https://facebook.github.io/react/docs/component-api.html#setstate

@philikon
Copy link

Do you have any ideas about what to do with React.render which also accepts a callback but also returns a value?

Two solutions, which aren't mutually exclusive:
(a) the callback gets the component as an argument / the promise returns the component. This will work for all cases where the component value is not needed synchronously.
(b) similarly to how ref={...} takes a function, the caller can supply a 2nd callback that gets called with the component instance, right after ReactDOM.render is done. Though in that case, one would wonder why not simply require that the consumer of ReactDOM.render add a ref={...} prop to the element themselves. In other words:

let el = ReactDOM.render(<Foobar />, div);
doSomethingWith(el);

becomes

let el;
ReactDOM.render(<Foobar ref={ref => el = ref} />, div);
doSomethingWith(el);

This is assuming that ref callbacks get called synchronously here.

@Azerothian
Copy link

setStateMixin using bluebird. It creates . this.setStateAsync for the current context,
I wanted to run promisifyAll on the React prototype but it seems that React.createClass hides its prototypes which is sad.

import Promise from "bluebird";

export default {
  componentWillMount() {
    this.setStateAsync = Promise.promisify(this.setState);
  },
};
import React from "react";
import Promise from "bluebird";

// ES6 class - have not tested this example (wrote it in the comment here) but should work
export default class Test extends React.Component {
  constructor() {
    super();
    this.setStateAsync = Promise.promisify(this.setState);
  }
}

so far this has worked fine in the situation of e.g.

{
  handleDoSomething() {
    return this.setStateAsync({
      loading: true,
    }).then(this.loadSomething).then((result) => {
      return this.setStateAsync({result, loading: false});
    });
  }
}

This is really dirty, it will promisify the current context functions that includes reacts and your own. only use it when your lazy and not in production.

import Promise from "bluebird";

export default {
  componentWillMount() {
    Promise.promisifyAll(this);
  },
};

@tobias-zucali
Copy link

@philikon

let el = ReactDOM.render(<Foobar />, div);
doSomethingWith(el);

becomes

let el;
ReactDOM.render(<Foobar ref={ref => el = ref} />, div);
doSomethingWith(el);

Wouldn't it be correct if it becomes

ReactDOM.render(
    <Foobar />
).then((foobarRef) => {
    doSomethingWith(foobarRef);
}).catch((error) => {
    logCatchedRenderingError(error);
});

This way I have the opportunity to

  • work with an ref as soon as the element was rendered
  • catch rendering errors on specific components ... currently the whole app explodes if one component throws an error during rendering ...
  • use promises for other functions (setState) as well

@pcwinters
Copy link

👍 getting promises for state based changes would be a big plus for shallow testing component lifecycles

@ConAntonakos
Copy link

Would this be syntactically sound?: Promise.resolve( this.setState({ ... }) );

Using the bluebird Promise library.

@arisAlexis
Copy link

+1 for this. since there is the possibility of a callback then it should be working with a promise. If it's an antipattern then remove the callback too.

@caseys
Copy link

caseys commented Jul 13, 2016

:+ IF it doesn't add too much bloat.

@spectrox
Copy link

spectrox commented Aug 1, 2016

How about this?

class AbstractComponent extends React.Component {
    setStatePromise(newState) {
        return new Promise((resolve) => {
            this.setState(newState, () => {
                resolve();
            });
        });
    }
}

It's very useful to have an option to use promises instead of callbacks.

@syranide
Copy link
Contributor

syranide commented Aug 1, 2016

@spectrox Or you can avoid introducing a new base class entirely:

function setStatePromise(that, newState) {
    return new Promise((resolve) => {
        that.setState(newState, () => {
            resolve();
        });
    });
}

@spectrox
Copy link

spectrox commented Aug 1, 2016

I already had one, so, it was the simplest solution to add such function in it. And it's hard to add helper function (or mixin) in an ES6 environment, where every js file works in it's own scope, you'll have to import it everytime.
So, it depends on an environment which option to choose 👍

@superddr
Copy link

superddr commented Aug 8, 2016

+1 , I'd love the promise way.

@f0rmat1k
Copy link

f0rmat1k commented Aug 20, 2016

Guys support es5, so i don't think they will add it before moving to es6. syranide's solution is well.

@peteychuk
Copy link

PR #8282 related to this issue.

@gaearon
Copy link
Collaborator

gaearon commented Jan 23, 2017

Not yet, but it will be async by default eventually.
However it is already async now in some cases.

@aweary
Copy link
Contributor

aweary commented Jan 23, 2017

@ConAntonakos check out this section in the documentation: State Updates May Be Asynchronous

@nhunzaker
Copy link
Contributor

nhunzaker commented Jan 23, 2017

What are the specific unknowns about returning a promise from setState? Just thinking:

  1. Committing to Promises. Does this rule out possible features?
  2. What actually triggers resolve/reject? Flushing of the update queue, or the individual setState transaction?
  3. Are there performance issues with producing a promise for every setState call that might otherwise never get used?

For the last item: what if setState returned an object that implemented the Promise public API? It wouldn't create a promise for every setState call, only if .then() was invoked.

or, what if React.Component implemented .then, resolving whenever the component's state transaction queue flushed?

@gaearon
Copy link
Collaborator

gaearon commented Jan 23, 2017

Committing to Promises. Does this rule out possible features?

Synchronous resolve would be one. setState would still be synchronous in some cases.

Are there performance issues with producing a promise for every setState call that might otherwise never get used?

Yes. We need to "remember" to call that callback at the right point in time. So we have to keep a list of them for components that provided the callback. However with Promises we'd have to always do that for every single update just in case somebody happens to use the promise.

For the last item: what if setState returned an object that implemented the Promise public API? It wouldn't create a promise for every setState call, only if .then() was invoked.

Seems awkward. Still an allocation, and you can't avoid queueing because somebody might then a Promise after it's completed. So we can't avoid actually storing the callback for every call.

Why do we need to do this at all? What do you gain by using Promises here? It's a bit of a "fringe" feature anyway that should be used for corner cases. Normally you should use lifecycle methods. Why add more complexity to it?

@nhunzaker
Copy link
Contributor

nhunzaker commented Jan 23, 2017

Why do we need to do this at all? What do you gain by using Promises here? It's a bit of a "fringe" feature anyway that should be used for corner cases. Normally you should use lifecycle methods. Why add more complexity to it?

FWIW, my vote is to not return a Promise from setState. All of the concerns you've mentioned are good, concrete reasons.

@ConAntonakos
Copy link

Thanks, @gaearon and @aweary. I wasn't fully aware that you could use setState like so:

this.setState((prevState, props) => ({
  counter: prevState.counter + props.increment
}));

@peacechen
Copy link

+1 for setState returning a promise. Consider this scenario:

functionA() sets state of someArray
functionB() sets state of other values in someArray

They are split into two functions due to business logic reasons. Sometimes A happens, sometimes B happens, sometimes both happen.

When A and B both happen and both functions are called, a race condition ensues. functionA mutates someArray and setState takes some time to update it. functionB comes in, gets the stale value of someArray and stomps over functionA's changes. Other times B happens first, immediately followed by A.
The current work-arounds are not ideal: callback hell, or smash both A and B into one monolithic function.

@gaearon
Copy link
Collaborator

gaearon commented Jan 27, 2017

@peacechen

There is already an API for this use case: this.setState(fn). The function will receive the current (never stale) values of the state and props as arguments, and should return the next state. It is safe to call setState(fn) several times.

For example:

function increment(prevState) {
  return {
    value: prevState.value + 1
  }
}

function multiply(prevState) {
  return {
    value: prevState.value * 5
  }
}

// inside the component
this.setState(increment);
this.setState(multiply);

@peacechen
Copy link

@gaearon Thanks, I am aware of the callback that setState supports. My request is to add support for promises to setState to avoid callback hell.

@ctrlaltdylan
Copy link

Just want to join the chorus of and agree that at least returning a Promise on setState would be nice to have because there are times you don't want to use componentDidUpdate because it could make your component render recursively for-ev-er.

For now I'm:

promisedSetState = (newState) => {
    return new Promise((resolve) => {
        this.setState(newState, () => {
            resolve()
        }
    }
}

@lutherism
Copy link

lutherism commented May 11, 2017

@gaearon I think the react team is missing why this issue is so popular.

Put yourself in the shoes of a JS developer learning ES6 and all the new libraries, Express, Gulp, etc. They've just started to understand the beauty of futures, streams, and async/await and take pride in 'knowing' how to avoid 'callback hell' which so many of their peers decry as the downfall of JS.

Then they start using React heavily, and find out that a core async part of the API uses callbacks! Which they were just done learning about how bad they are!

This one small issue is going to keep being a major pain point for new React developers, until either A) Promises are no longer used ubiquitously B) setState is longer used asynchronously or C) React returns a Promise from setState.

@peacechen
Copy link

There are working examples of libraries that have been enhanced to support promises while retaining backwards compatibility with callbacks. In the case of setState, if a callback param is provided, it would call that as usual. However if no callback is passed in, setState would return a promise. The change is straight-forward, similar to @ctrlaltdylan 's snippet with an added check for the callback param.

@riverleo
Copy link

async () => {
  await this.setState({ ... });
  // do something
}

it works to me.

@peacechen
Copy link

@riverleo That's interesting. I understood that async/await works on Promise objects (it's syntactic sugar for Promise .then()). Can you confirm that it's actually waiting for the setState to complete before executing your code?

That pattern works for the simple case, but there are many times where I have a function that calls setState within, and I want to wait for that function to complete the setState call. For example:

function myFunction() {
  // Do something
  this.setState({...});
}

// would like to do this to execute after setState completes:
myFunction.then(...);

At the end of the day, we're trying to find work-arounds for a fundamental deficiency in setState(). The "right" way would be to add Promise capability to it.

@riverleo
Copy link

riverleo commented Jun 16, 2017

@peacechen The code below is the contents of the final build of the babel.

var _ref2 = (0, _asyncToGenerator3.default)(_regenerator2.default.mark(function _callee(paginate) {
  return _regenerator2.default.wrap(function _callee$(_context) {
    while (1) {
      switch (_context.prev = _context.next) {
      case 0:
        _context.next = 2;
        return _this.setState({ ... });

      case 2:
        // do something

      case 3:
      case 'end':
        return _context.stop();
      }
    }
  }, _callee, _this2);
}));

And this is my .babelrc setting. specifically, the babel-plugin-syntax-async-generators plug-in included in the stage-0 configuration enables this.

{
  "presets": [
    "flow",
    "next/babel",
    "es2015",
    "stage-0",
  ],
  "plugins": [
    "transform-runtime",
    "transform-flow-strip-types",
    ["styled-components", { "ssr": true, "displayName": false, "preprocess": true } ],
    ["react-intl", { "messagesDir": "./locale/.messages/" }],
  ],
}

I checked the log to see if it worked, and it worked as expected with no problems to me.

@milesj
Copy link
Contributor

milesj commented Jun 16, 2017

@riverleo await wraps the expression in Promise.resolve. This is not correct, as the promise is not resolving once the state has updated.

@peacechen
Copy link

peacechen commented Jun 16, 2017

@riverleo , I agree with @milesj , the generated code shows that it's not properly waiting for setState to complete. Your app just happens to finish setState quickly in that case, but will not for all cases.
Line 7 of the generated code is the key:

return _this.setState({ ... });

The correct way would be

_this.setState({ ... }, () => resolve returnVal);

(the code around that would be different, for example resolve would need to be present from a created Promise).

Created PR #9989 that returns a promise if no callback param is provided to setState. This was much simpler than diving into React's callback queue. Modifying the queue would require re-architecting and a significant rewrite.

@gaearon
Copy link
Collaborator

gaearon commented Jun 16, 2017

Wrote more thoughts on why I think we won't be supporting this in #9989 (comment).

It is not very plausible that we'll change this anytime soon for these reasons so I'll close the issue.

We do, however, plan to look into providing better component APIs for components and state next year. So we'll consider this feedback as part of our future work.

Thanks!

@johnculviner
Copy link

johnculviner commented Sep 13, 2017

I look forward to seeing this added next year. It is a common paradigm to support both callbacks and promises but usually not just callbacks on 'up-to-date' libraries.

The situation for having dependent set states does exist (and IMO not just an anti-pattern) which is why the callback is there in the first place.

Tossing stuff in componentDidUpdate certainly does "get around" this but you also end up with "cause here" & "effect somewhere else" sprinkled all over your React component rather than right next to each other creating a bit of a maintenance issue (at least a little confusing I'd say to anyone coming back and looking at the code next week)

@milad1367
Copy link

milad1367 commented Dec 13, 2017

I use this code but get this error:
ExceptionsManager.js:65 Cannot read property 'then' of undefined

componentDidMount() {
 FirebaseApp.database().ref('users').on("value",(snap)=>{
   var items = [];
    snap.forEach((child)=>{
      items.push({
        name:child.val().username
      })
    })
    this.setState({
      users:items
    }).then(()=>{
      users2 = this.state.users.map((item)=>{
       <Text> {item} </Text>

     })
    });
 })

pls help me.

@ConAntonakos
Copy link

@milad1367 As far as I know, this.setState does not return a promise, so you can't chain a .then unless you Promisify it. You're better off using the second argument, or callback function.

this.setState({
    users:items
}, () => {
    let users2 = this.state.users.map((item)=>{
        <Text> {item} </Text>
    });
});

@milad1367
Copy link

tnx.i following these instructions.

@kevin-daniel-hunt
Copy link

I have a question regarding setState and why I end up using the callback function often.

My understanding is that setState is asyncronous. In my product we do a lot of async ajax calls. While the call is happening I setState({isFetching: true}); This value is used to determine if the user sees a spinner or fetched data. When the request is complete and the data is parsed, the value is changed to false, which then tells the render function to display the data and not a spinner.

Within my fetch() function, which submits the request or refreshes the data I typically have a scheme like this:

this.setState({
    isFetching: true,
}, () => {
    $.ajax({
        url: '/data'
    })
    .done((data) => {
        this.setState({
            isFetching: false,
            data
        });
   });
});

This seems to me to be a common use case for using the callback in setState. I know I have seen people do things like this instead:


this.setState({
    isFetching: true,
});

$.ajax({
    url: '/data'
})
.done((data) => {
    this.setState({
        isFetching: false,
        data
    });
});

But because of the async nature of setState can we ever truly be sure the first setState won't finalize after the second (yes in theory this will almost never happen but is it still possible?)

@gaearon
Copy link
Collaborator

gaearon commented Dec 15, 2017

I look forward to seeing this added next year.

We're not adding this next year (as I said above). I said that we'll consider this feedback as we think about next APIs, not that we'll make setState return a Promise.

Tossing stuff in componentDidUpdate certainly does "get around" this but you also end up with "cause here" & "effect somewhere else" sprinkled all over your React component rather than right next to each other creating a bit of a maintenance issue (at least a little confusing I'd say to anyone coming back and looking at the code next week)

In my experience, it’s the opposite pattern that becomes a maintenance burden. Somebody adds a setState callback in one place because that’s where the state gets set. Then somebody else calls setState in another place but doesn’t realize that the component’s correctness depends on that callback code running after the state change.

Putting this code in componentDidUpdate turns your component into something closer to a "state machine". The component behaves more predictably when the side effects are based on explicit conditions (e.g. a prop or a state field has changed) rather than on where in the code you happened to call setState. I understand it is a little bit frustrating if that’s not a programming model you’re used to, but I encourage you to give this a try.

The situation for having dependent set states does exist (and IMO not just an anti-pattern) which is why the callback is there in the first place.

Yes, it exists, and it is very rare (e.g. setting a focus or triggering an animation). However, for the case where it is necessary you want all the flexibility. Promises are not flexible as they force execution to happen on next tick. We could return a "thenable" object that looks like a Promise. But that's not what this issue was proposing. We recently set up an RFC process for proposing changes to React. You're welcome to propose this: https://github.com/reactjs/rfcs.

But because of the async nature of setState can we ever truly be sure the first setState won't finalize after the second (yes in theory this will almost never happen but is it still possible?)

No, it's not ever possible, React keeps an internal queue of setState calls to prevent exactly such problems. Later calls always override earlier calls. It is always safe to do as you show in the second example. You don't need to wait for setState to flush to start your API call.

I'm going to lock this issue because discussing in closed issues is not very productive. As I mentioned earlier in this comment, if you feel strongly that this is a valuable proposal despite its drawbacks, you are welcome to submit an RFC: https://github.com/reactjs/rfcs. Thank you!

@facebook facebook locked and limited conversation to collaborators Dec 15, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests