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

Quick update of atom with a previous value is ignored #24

Closed
lisss opened this issue Jan 11, 2018 · 5 comments
Closed

Quick update of atom with a previous value is ignored #24

lisss opened this issue Jan 11, 2018 · 5 comments

Comments

@lisss
Copy link

lisss commented Jan 11, 2018

When you quickly update an atom with a previous value right after updating with a new one - the last change is ignored
The code sample can be found here

@valentyng
Copy link
Collaborator

It looks like the issue is here:

According to the documentation (https://reactjs.org/docs/react-component.html#setstate):

Think of setState() as a request rather than an immediate command to update the component.

liftedComponent.setState() doesn't update liftedComponent.state immediately.

So, every new renderCache is compared to the initial value, and if they are equal, the state isn't updated.

@valentyng
Copy link
Collaborator

valentyng commented Jan 11, 2018

A possible solution is moving structEq(liftedComponent.state.renderCache, renderCache) to shouldComponentUpdate (https://github.com/grammarly/focal/blob/master/src/react/react.ts#L116).

However, I'm not sure about the performance here.
@blacktaxi what do you think?

@blacktaxi
Copy link
Member

That seems to be a good catch! Let me take some time to debug this. Not sure about moving structEq to shouldComponentUpdate, intuitively it indeed seems to ought to affect performance.

Another obvious solution could be to use the this.setState(prevState => ...) overload and perform the structEq call in the callback.

@oleksiilevzhynskyi
Copy link
Collaborator

@blacktaxi this.setState(prevState => prevState) triggers render. It looks like it doesn't make sense to use structEq here. Code - https://codesandbox.io/s/0ol9q3q38w

@blacktaxi
Copy link
Member

Added a repro and tracking this in #27 now

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

No branches or pull requests

4 participants