Updates to values prop in onChange callback are ignored

[RookY2K]

I've got a use-case that doesn't seem well supported by react-dropdown-select(RDS). I am attempting to use RDS as the base for a Tree component, but there seems to be some inconsistencies with how the component handles prop updates under the hood. I've got a codesandbox that reproduces the issue that I'll spell out below.

https://codesandbox.io/s/rds-values-prop-inconsistent-fkpr23

In this setup, A1 and A2 are considered children of A.

Desired interactions: 1) Select A -> A1 and A2 become selected 2) Do (1) then Deselect A -> Deselects A1 and A2 3) Do (1) then Deselect A1 or A2 -> Deselects A

Actual interactions: 1) Does what is desired 2) Only deselects A and leaves A1 and A2 selected 3) Only deselects A1 or A2 and leaves A and the other child selected

I added some simple console.log statements that show the individual processing steps. I also added a console.warn to show what the actual value of the values prop is immediately prior to setting it as a prop for RDS. For both interactions (2) and (3), the values prop has the correct value.

Screenshots (2)

(3)

[sanusart]

Aah. I can see how this one might become a challenge for RDS. I would assume some internals only consider flat array and not nested with children. I will check options for that.

[RookY2K]

Aah. I can see how this one might become a challenge for RDS. I would assume some internals only consider flat array and not nested with children. I will check options for that.

TL;DR, the "nested-ness" of the options isn't what is causing this bug. See the following comment for a better demo of the problem: https://github.com/sanusart/react-dropdown-select/issues/260#issuecomment-1358382693

The "nested-ness" of the options can be considered an implementation detail. The options sent into the RDS component are actually flat. There is just a children attribute on any option with children. There is no expectation that RDS knows what to do with that attribute and it should be just ignored.

I overrode the compareValuesFunc in the above linked codesandbox just to be sure and the same problems are present. In the override, I made sure that the comparison only cared about the value param.

Here is a screenshot of the relevant console output (I added output for the compareValuesFunc function:

[RookY2K]

Also, just to be doubly certain, I removed the children attribute from A and just hard-coded some of the utility functions with the correct parent / child relationships. So RDS is only receiving flat options with no nested options. The problem still exists.

[RookY2K]

Based on the ordering of the console logs, here is what I think is happening:

1. De-selecting A causes two things to happen within the same update cycle:
   1. A is removed from internal state leaving A1 and A2
   2. onChange is processed by consuming app code and sets the values prop to an empty array
2. In componentDidUpdate L120, the conditional fails because prevProps.values/this.props.values are different but so are prevProps.values and prevState.values

There is an inbuilt assumption that props changes won't happen right after a state change. I.E. The updated values won't be modified in the onChange callback. The root of this problem, as far as I can see it, is that I'm modifying the selectedValues in the handleChange callback and RDS can't handle that. I'm going to put together a simplified sandcodebox that should demonstrate this bug and post a followup comment.

[RookY2K]

Here is the simplified codesandbox that more succinctly demonstrates the issue: https://codesandbox.io/s/rds-onchange-issue-ri5zyo

The basic setup is that in the handleChange callback definition, I add the third option to the updated values if it isn't already there. So, the expectation is that whatever option is picked should also cause the third option to be picked. In this example, the third option is Option D.

The screenshot I'm going to post below shows what happens if I choose Option A. Option A gets selected, but you can see that the provided values prop is Option A and Option D which is not reflected in the RDS component.

RDS internal props / state

[sanusart]

Isn't it very similar situation to what showed in this example?

https://react-dropdown-select.netlify.app/examples#Custom-Content-And-Dropdown-renderers

Here are several different options selected with one hit. 1st, 2nd and 4th.

On Mon, Dec 19, 2022, 11:20 PM RookY2K @.***> wrote:

Here is the simplified codesandbox that more succinctly demonstrates the issue: https://codesandbox.io/s/rds-onchange-issue-ri5zyo

The basic setup is that in the handleChange callback definition, I add the third option to the updated values if it isn't already there. So, the expectation is that whatever option is picked should also cause the third option to be picked. In this example, the third option is Option D.

The screenshot I'm going to post below shows what happens if I choose Option A. Option A gets selected, but you can see that the provided values prop is Option A and Option D which is not reflected in the RDS component. [image: image] https://user-images.githubusercontent.com/12241742/208526016-4b19c22c-fd47-4dd7-8043-0964d1f42bb8.png

RDS internal props / state [image: image] https://user-images.githubusercontent.com/12241742/208526159-abc59c67-386d-42cb-91c4-c0a7671c894a.png

— Reply to this email directly, view it on GitHub https://github.com/sanusart/react-dropdown-select/issues/260#issuecomment-1358382693, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACBLRT6HVE5SLX4PY26WQDWODGR5ANCNFSM6AAAAAATDWTWR4 . You are receiving this because you commented.Message ID: @.***>

[RookY2K]

Here are several different options selected with one hit. 1st, 2nd and 4th

The issue isn't with selecting multiple options at one time. The issue is updating the values prop in the onChange callback. Any update to values in the onChange handler will be effectively ignored.

The cause of the issue is this block in the componentDidUpdate method:

if (
  !this.props.compareValuesFunc(prevProps.values, this.props.values) &&
  this.props.compareValuesFunc(prevProps.values, prevState.values)
) {
  this.setState(
    {
      values: this.props.values
    },
    () => {
      this.props.onChange(this.state.values);
    }
  );
  this.updateSelectBounds();
}

!this.props.compareValuesFunc(prevProps.values, this.props.values)

The first predicate in the conditional will evaluate to true as expected since the props changed.

this.props.compareValuesFunc(prevProps.values, prevState.values)

The second predicate, however, evaluates to false because prevState.values reflects the values that triggered the onChange whereas prevProps.values reflects the last values that were passed into the RDS component prior to the onChange triggering.

This leads to a valid prop change not being handled by RDS.

[sanusart]

I think it is actually valid. First only compares with props and the second compares prop with state. I need to recreate in order to see the where is the described race condition happened. Prev. state is a state before change.

On Tue, Dec 20, 2022, 8:30 AM RookY2K @.***> wrote:

Here are several different options selected with one hit. 1st, 2nd and 4th

The issue isn't with selecting multiple options at one time. The issue is updating the values prop in the onChange callback. Any update to values in the onChange handler will be effectively ignored.

The cause of the issue is this block in the componentDidUpdate method:

if ( !this.props.compareValuesFunc(prevProps.values, this.props.values) && this.props.compareValuesFunc(prevProps.values, prevState.values) ) { this.setState( { values: this.props.values }, () => { this.props.onChange(this.state.values); } ); this.updateSelectBounds(); }

!this.props.compareValuesFunc(prevProps.values, this.props.values)

The first predicate in the conditional will evaluate to true as expected since the props changed.

this.props.compareValuesFunc(prevProps.values, prevState.values)

The second predicate, however, evaluates to false because prevState.values reflects the values that triggered the onChange whereas prevProps.values reflects the last values that was passed into the RDS component prior to the onChange triggering.

That means that a valid prop change isn't handled by RDS.

— Reply to this email directly, view it on GitHub https://github.com/sanusart/react-dropdown-select/issues/260#issuecomment-1358888878, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACBLRQTTQJCJJYA3BE4LNLWOFG7VANCNFSM6AAAAAATDWTWR4 . You are receiving this because you commented.Message ID: @.***>

[RookY2K]

To be clear, this is a bug. RDS should be able to handle onChange updating the values prop. It cannot due to the code block I highlighted above. Here is an example flow:

const options = [
  { label: 'Option A', value: 'A' },
  { label: 'Option B', value: 'B' },
  { label: 'Option C', value: 'C' },
  { label: 'Option D', value: 'D' }
];

// Pertinent RDS onComponentUpdate block
if (
  !this.props.compareValuesFunc(prevProps.values, this.props.values) &&
  this.props.compareValuesFunc(prevProps.values, prevState.values)
) {
  this.setState(
    {
      values: this.props.values
    },
    () => {
      this.props.onChange(this.state.values);
    }
  );
  this.updateSelectBounds();
}

1. User selected 'Option A'
2. Current state of RDS is updated to look like this:

this.state = {values: [{label: 'Option A', value: 'A'}]

3. RDS consumer gets onChange event with updated values being [{label: 'Option A', value: 'A'}]
4. RDS consumer updates (via state update) values in onChange to be [{label: 'Option A', value: 'A'}, {label: 'Option C', value: 'C'}]
5. RDS gets updated props that look like

this.props = {values: [{label: 'Option A', value: 'A'}, {label: 'Option C', value: 'C'}]

6. In the onComponentDidUpdate block I highlighted, here are the values of the pertinent variables:

this.props.values = [{label: 'Option A', value: 'A'}, {label: 'Option C', value: 'C'}]
prevProps.values = []
prevState.values = [{label: 'Option A', value: 'A'}]

7. When comparing this.props.values and prevProps.values, they are not equal, so the first predicate is true, as desired.
8. When comparing prevProps.values and prevState.values, they are also not equal, so the second predicate is false, which is not desired.
9. Due to this, RDS state is not updated with this.props.values and the passed in values prop is effectively ignored.

[sanusart]

But this block is getting triggered only on very particular situations. I dont understand how it is happening. I mean this would never work, why now?

On Wed, Dec 21, 2022 at 5:25 PM RookY2K @.***> wrote:

To be clear, this is a bug. RDS should be able to handle onChange updating the values prop. It cannot due to the code block I highlighted above. Here is an example flow:

const options = [ { label: 'Option A', value: 'A' }, { label: 'Option B', value: 'B' }, { label: 'Option C', value: 'C' }, { label: 'Option D', value: 'D' }]; // Pertinent RDS onComponentUpdate blockif ( !this.props.compareValuesFunc(prevProps.values, this.props.values) && this.props.compareValuesFunc(prevProps.values, prevState.values)) { this.setState( { values: this.props.values }, () => { this.props.onChange(this.state.values); } ); this.updateSelectBounds();}

1. User selected 'Option A'
2. Current state of RDS is updated to look like this:

this.state = {values: [{label: 'Option A', value: 'A'}]

1. RDS consumer gets onChange event with updated values being [{label: 'Option A', value: 'A'}]
2. RDS consumer updates (via state update) values in onChange to be [{label: 'Option A', value: 'A'}, {label: 'Option C', value: 'C'}]
3. RDS gets updated props that look like

this.props = {values: [{label: 'Option A', value: 'A'}, {label: 'Option C', value: 'C'}]

1. In the onComponentDidUpdate block I highlighted, here are the values of the pertinent variables:

this.props.values = [{label: 'Option A', value: 'A'}, {label: 'Option C', value: 'C'}]prevProps.values = []prevState.values = [{label: 'Option A', value: 'A'}]

1. When comparing this.props.values and prevProps.values, they are not equal, so the first predicate is true, as desired.
2. When comparing this.props.values and prevState.values, they are also not equal, so the second predicate is false, which is not desired.
3. Due to this, RDS state is not updated with this.props.values and the passed in values prop is effectively ignored.

— Reply to this email directly, view it on GitHub https://github.com/sanusart/react-dropdown-select/issues/260#issuecomment-1361482688, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACBLRVQJFNSK6GFJEQMOGLWOMONRANCNFSM6AAAAAATDWTWR4 . You are receiving this because you commented.Message ID: @.***>

[sanusart]

I actually think the bug is on the other block. And the bug is the way props being set - e.g. not in as the callback of setState.

[sanusart]

@RookY2K I looked at it a bit from the internal state perspective. I think it is not able to update because react is guarding itself from doing update while other update is in progress. Meaning one is trying to update state while the state is still updating which is triggering another update.

This is very close to an infinit loop. I would approach this from a different angle. If you need such functionality as a nested tree - maybe a custom renderrer is better fit. It is exposing methods wich allows you to handle state directly.

I'll bee looking into this in any case to see if safe solution can be found.

[RookY2K]

For the record, I've already developed a workaround to have everything update as expected for my use case. I just wanted to make sure that the bug was brought to your attention. My naive approach to fixing this bug would be to remove the second predicate: this.props.compareValuesFunc(prevProps.values, prevState.values)

But I haven't tested that, and I'm sure there are multiple reasons why that would be a bad idea. This would probably be easier to handle in a FunctionComponent using useEffect hooks rather than relying on the componentDidUpdate, but that seems like a pretty large rewrite.

[RookY2K]

Also, to be clear, this is not an internal react issue. Due to how RDS works, clicking an option updates values in state. It isn't until that state update has completed that the onChange event is triggered. Therefore, when the next prop update occurs as a response to the onChange, prevState will now be equivalent to this.state in RDS.

[sanusart]

Bug on RDS or not, I never said it was an issue on react, I think it is prevention of loop. I might be wrong. But updating state and triggering update on the same time sounds like something react would try to prevent and optimize.

selectAll method for example can be used to update different options manipulated on the fly.

As I mentioned I will check how we can properly feed RDS updates back safely. If you have any specific suggestions, I am obviously open to hear them and implement.

[sanusart]

Functional component rewrite indeed will be a huge change. But this is something that will need to happened. Eventually they will stop supporting lifecycles I guess.

[sanusart]

I just wanted to make sure that the bug was brought to your attention

And I appreciate it.

[RookY2K]

Bug on RDS or not, I never said it was an issue on react, I think it is prevention of loop. I might be wrong. But updating state and triggering update on the same time sounds like something react would try to prevent and optimize.

My apologies, I didn't mean to put words into your mouth.

This is getting to the level of pedantic, but I just wanted it to be clear that this wasn't react optimizing or preventing loops. In fact, it seems like it is actually RDS that is attempting to prevent a loop. React does have heuristics to optimize how and when state is updated, but none (that I know of) to prevent a loop. In this case, the bug happens after state is updated in RDS.

In any event, you are on the case, so I'll let you do your thing. Thank you for looking into this.

[sanusart]

All is good 😊

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.