-
Notifications
You must be signed in to change notification settings - Fork 46.8k
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
Comments
Do you have any ideas about what to do with There are a few other techniques that can be used. You can have the row's 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 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? |
In my experience, whenever I'm tempted to use |
I was wondering the same. Thanks @gaearon for the trick it works as expected. |
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. |
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. |
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. |
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 |
what's a setState callback XD @gaearon |
@zzz6519003 It's a callback that can be passed as the second argument to |
Two solutions, which aren't mutually exclusive: 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 |
setStateMixin using bluebird. It creates . this.setStateAsync for the current context, 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);
},
}; |
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
|
👍 getting promises for state based changes would be a big plus for shallow testing component lifecycles |
Would this be syntactically sound?: Using the |
+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. |
:+ IF it doesn't add too much bloat. |
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. |
@spectrox Or you can avoid introducing a new base class entirely: function setStatePromise(that, newState) {
return new Promise((resolve) => {
that.setState(newState, () => {
resolve();
});
});
} |
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. |
+1 , I'd love the promise way. |
Guys support es5, so i don't think they will add it before moving to es6. syranide's solution is well. |
PR #8282 related to this issue. |
Not yet, but it will be async by default eventually. |
@ConAntonakos check out this section in the documentation: State Updates May Be Asynchronous |
What are the specific unknowns about returning a promise from setState? Just thinking:
For the last item: what if or, what if React.Component implemented |
Synchronous resolve would be one.
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.
Seems awkward. Still an allocation, and you can't avoid queueing because somebody might 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 |
+1 for setState returning a promise. Consider this scenario:
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. |
There is already an API for this use case: 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); |
@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. |
Just want to join the chorus of and agree that at least returning a Promise on For now I'm:
|
@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. |
There are working examples of libraries that have been enhanced to support promises while retaining backwards compatibility with callbacks. In the case of |
async () => {
await this.setState({ ... });
// do something
} it works to me. |
@riverleo That's interesting. I understood that async/await works on Promise objects (it's syntactic sugar for 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:
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. |
@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 {
"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. |
@riverleo |
@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.
The correct way would be
(the code around that would be different, for example 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. |
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! |
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) |
I use this code but get this error:
pls help me. |
@milad1367 As far as I know,
|
tnx.i following these instructions. |
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 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:
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?) |
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
In my experience, it’s the opposite pattern that becomes a maintenance burden. Somebody adds a Putting this code in
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.
No, it's not ever possible, React keeps an internal queue of 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! |
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.
The text was updated successfully, but these errors were encountered: