tomchentw / react-toastr

React.js toastr component
https://tomchentw.github.io/react-toastr/
MIT License
619 stars 112 forks source link

State set race conditions #64

Open NullDivision opened 8 years ago

NullDivision commented 8 years ago

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 commented 8 years ago

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

plemarquand commented 8 years ago

@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 commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

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.