react-toastr icon indicating copy to clipboard operation
react-toastr copied to clipboard

State set race conditions

Open NullDivision opened this issue 8 years ago • 5 comments

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.

NullDivision avatar Jul 06 '16 13:07 NullDivision

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

RangarajKaushikSundar avatar Jul 07 '16 19:07 RangarajKaushikSundar

@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.

plemarquand avatar Jul 11 '16 17:07 plemarquand

@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.

RangarajKaushikSundar avatar Jul 11 '16 17:07 RangarajKaushikSundar

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.

NullDivision avatar Jul 11 '16 18:07 NullDivision

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.

RangarajKaushikSundar avatar Jul 19 '16 18:07 RangarajKaushikSundar