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

Synchronous setState #1496

Open
qostya opened this issue Nov 9, 2019 · 15 comments
Open

Synchronous setState #1496

qostya opened this issue Nov 9, 2019 · 15 comments
Labels

Comments

@qostya
Copy link

qostya commented Nov 9, 2019

Why does Inferno's setState is synchronous?

How it works now (v7.3.2):

Снимок экрана 2019-11-09 в 18 39 13

Result:

Снимок экрана 2019-11-09 в 18 40 11

How it works in React:

Снимок экрана 2019-11-09 в 17 48 52

@Havunen
Copy link
Member

Havunen commented Nov 10, 2019

Because originally React was that way too. I think they changed it in v16. Inferno starts queuing Set state calls when you chain them. When this queuing process is in use then state changes get merged together. And it behaves like in React now.

@qostya
Copy link
Author

qostya commented Nov 11, 2019

So how can I chain them? Nothing works here...

class Abc extends Component {
  state = {};
  handleChange = (e: any) => {
    console.log(1);
    this.setState(() => ({a: e.target.value}));
    this.setState(() => ({a: e.target.value}));
    console.log(3);
  };

  render() {
    console.log(2);
    return <input type="text" value={this.state.a} onInput={this.handleChange} />;
  }
}

Prints

1
2
2
3

Parent-to-child async setState also is not working:

class Abc extends Component {
  state = {};

  handleChange = (e: any) => {
    this.props.onChange(e.target.value);
    this.setState({a: e.target.value});
  };

  render() {
    console.log(2);
    return <input type="text" value={this.state.a} onInput={this.handleChange} />;
  }
}


class Cba extends Component {
  state = {};

  handleChange = (a: any) => {
    this.setState({a});
  };

  render() {
    return <Abc onChange={this.handleChange} />;
  }
}

<Cba />

Prints 2 2 times

@qostya
Copy link
Author

qostya commented Nov 11, 2019

I understand that it triggers only in lifecycle methods... Do you have any plans to expand that to event handlers?

@Havunen
Copy link
Member

Havunen commented Nov 11, 2019

Yeah that could be done. There might be some edge cases that needs to handled, but it should be doable

@Havunen
Copy link
Member

Havunen commented Nov 12, 2019

@qostya Do you have some arguments why it should do "async" -setState flow from delegated event handlers? Is it because React does so or is there another argument? What If React changes that, do we change then again?

@Havunen
Copy link
Member

Havunen commented Nov 12, 2019

IMO it would be much nicer to have async flow or sync flow always, but that is not compatible with React either.

@qostya
Copy link
Author

qostya commented Nov 12, 2019

@Havunen I think it's not just for React, but because we need the same and reusable api everywhere

@Havunen
Copy link
Member

Havunen commented Nov 13, 2019

@qostya Can you clarify what means everywhere in this context? Are you referring to infernojs APIs or other libraries?

@robbiespeed
Copy link

Is the reason for async mainly so that state changes get queued?
I agree with @Havunen it would be nice if it was just simplified so setState was either always async or sync. Then another method could be added either setStateAsync or setStateSync to give the user control as to what to use.

@Havunen
Copy link
Member

Havunen commented Dec 9, 2019

@robbiespeed Yes, when it goes into "async" flow it will start merging state changes together to avoid diffing whole vNode tree multiple times. Its more about batching the updates together.

I think React does it that way too, only exception is that how event handlers are handled. See React setState is synchronous here:
https://jsfiddle.net/xaL8p1c6/

This version of fiddle is async because setState is triggered from event handler, like in the original post
https://jsfiddle.net/z40kvpca/

@robbiespeed
Copy link

I understand, it seems like a very bad design if you can't be sure whether it's going to be async or sync. That should be chosen within the method, not left to be determined based on call site.

@tamb
Copy link

tamb commented Apr 29, 2020

Can we get a code example of async setState from chaining?

@tamb
Copy link

tamb commented Apr 29, 2020

Is the reason for async mainly so that state changes get queued?
I agree with @Havunen it would be nice if it was just simplified so setState was either always async or sync. Then another method could be added either setStateAsync or setStateSync to give the user control as to what to use.

I agree 100%. I would say since performance is so amazing to leave the implementation as is and then add two additional methods for sync and async.

@Havunen
Copy link
Member

Havunen commented Sep 19, 2020

Originally InfernoJS had setState and setStateSync but setStateSync was dropped due to problems it can cause. When the application grows one might accidentally enter into infinite loop using setStateSync version when those methods were merged into one at the same time setState was changed so that it automatically handles that infinite loop problem by queuing changes.

Can we get a code example of async setState from chaining?

In the previous post I added two jsFiddle examples from React where the order changes depending on how the code is called. Check console logs

@Havunen
Copy link
Member

Havunen commented Jun 19, 2022

This is something that I would like to implement for v9.
I believe the only correct way forward would be to make setState always "async". It cannot be always "sync" due to the infinite loop problem that it creates. It also has callback function parameter so its already an indication that it might not run synchronously.

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

No branches or pull requests

4 participants