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: #260 (comment)

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 <#260 (comment)>, 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 <#260 (comment)>, 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 <#260 (comment)>, 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.