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

State set race conditions #64

Open
NullDivision opened this issue Jul 6, 2016 · 5 comments
Open

State set race conditions #64

NullDivision opened this issue Jul 6, 2016 · 5 comments
Labels

Comments

@NullDivision
Copy link

When calling this.refs.toastContainer.clear() calls to setState() in ToastContainer::_handle_toast_remove() enter a race condition causing the state to reset instead of removing all elements.

The end result is the last toast being removed and the rest remaining on screen.

A solution would be to wait for the next digest cycle with something like this:

setTimeout(() => {
  this.state.toasts[operationName]((found, toast, index) => {
    if (found || toast.toastId !== toastId) {
      return false;
    }
    this.setState(update(this.state, {
      toasts: { $splice: [[index, 1]] },
    }));
    return true;
  }, false);
}, 0);

If memory serves the setState function uses an internal setTimeout to execute updates which would mean this.state will be the same value for all calls in the loop.

@RangarajKaushikSundar
Copy link
Collaborator

@NullDivision Thank you for notifying this. A fix will be provided for this ASAP.

@plemarquand
Copy link

plemarquand commented Jul 11, 2016

@NullDivision @RangarajKaushikSundar I think the correct pattern is:

this.setState((state) => {
 ...
 return update(state, {
    toasts: { $splice: [[index, 1]] },
  }));
});

This enqueues the state modification such that the result of the previous setState feeds in to this one. That was you can do a series of modifications using the last set state as input.

@RangarajKaushikSundar
Copy link
Collaborator

RangarajKaushikSundar commented Jul 11, 2016

@plemarquand @NullDivision

Well actually, this bug is because the toastr currently assumes that the default behaviour of it is to prevent duplicates. We just need one small condition check whether it is a clear all call before the splice. So as you mentioned, it would not splice one by one, but remove it completely.

@NullDivision
Copy link
Author

I agree with @plemarquand's solution. I don't see why you'd want to use the same state and rely on the index. I think a UID would solve a lot of problems for this case.

@RangarajKaushikSundar
Copy link
Collaborator

I agree! I am working on fixing these issues. If there are devs available to refactor code to be stable with latest versions of React, kindly comment here. Also, any suggestions on new features/ optimizations on existing functionalities are welcomed.

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

No branches or pull requests

3 participants