Skip to content

Conversation

@jamiebuilds
Copy link
Owner

This re-implements parts of setState so that it returns a promise It simplifies a lot about the implementation. The only problem is that we always pass a callback to React's setState which may be a perf issue for some apps.

@jamiebuilds
Copy link
Owner Author

cc @sindresorhus

@paul-phan
Copy link

Yes, I already facing some problem while using container.setState with controlled component and the setState callback not working sometimes. Seem like this implement will fix the callback issue.

return;
return Promise.all(promises).then(() => {
if (callback) {
return callback();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If something inside the callback function throws then the error might get swallowed.

onUpdate: Listener = cb => {
this.setState(DUMMY_STATE, cb);
onUpdate: Listener = () => {
return new Promise(resolve => {
Copy link

@xat xat Apr 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just return this.setState(DUMMY_STATE); (instead of creating a new promise)?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because then we dont wait for the callback

@sindresorhus
Copy link
Contributor

@jamiebuilds I tried this in our app and it works perfectly.

@ehaynes99
Copy link

Why not pick one or the other? There is no need for 2 different mechanisms to notify on update, and the promise version is cleaner and easier to test.

const callback = () => console.log('state updated')

// with callback
setState({
  some: 'newValue',
}, callback)

// with promise
setState({
  some: 'newValue',
}).then(callback)

// sooner or later, someone will do this
const promise = setState({
  some: 'newValue',
}, callback)
promise.then(callback)

@ldthorne
Copy link

@jamiebuilds Is there any update on this? I'd love to see this PR merged in -- it makes the setState API consistent with React's API. As others have pointed out, Promises are a much cleaner interface and personally, I'm hesitant to introduce (essentially mandatory) callbacks to my codebase.

@ehaynes99
Copy link

@ldthorne This isn't consistent, which is fine, but is different from the behavior of React. React's API does not return a promise, it uses the callback style.

React's API:

  • returns undefined
  • runs asynchronously
  • batches together multiple updates (i.e. multiple setState calls together will only rerender once)
  • allows an optional callback

Unstated since version 2:

This PR:

  • returns a promise that resolves to undefined
  • runs asynchronously
  • rerenders for every setState call
  • allows an optional callback

@jamiebuilds jamiebuilds merged commit 64b5d42 into master May 15, 2018
@jamiebuilds jamiebuilds deleted the promises branch May 15, 2018 22:53
@ldthorne
Copy link

@jamiebuilds can you create a new release for this on NPM? We'd love to start using this in production

@nfort
Copy link

nfort commented May 24, 2018

@jamiebuilds
Demo problem: https://codesandbox.io/s/0qlkn033v

What about this case?

@jamiebuilds
Copy link
Owner Author

@nfort fixed in 2.1.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants