Closed RookY2K closed 1 year ago
This pull request is automatically built and testable in CodeSandbox.
To see build info of the built libraries, click here or the icon next to each commit SHA.
Latest deployment of this branch, based on commit be0bcff85728edea738709f9757642818be2d225:
Sandbox | Source |
---|---|
react-dropdown-select | Configuration |
I believe this is needed when component being manipulated from outside.
I believe this is needed when component being manipulated from outside.
I'm a little uncertain what you mean by "manipulated from outside". As far as I can discern, there are two meaningful blocks in that componentDidUpdate
.
The first, the one I'm proposing the update for, checks if thevalues
prop has been updated
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); // seems unnecessary
}
);
this.updateSelectBounds();
}
This will set state with the updated values. Right now, it also calls the onChange
callback. However, after the state update, this section will get hit:
if (prevState.values !== this.state.values) {
this.props.onChange(this.state.values); // another call to onChange
this.updateSelectBounds();
}
The onChange
callback is invoked again.
I can't see a situation where that is desired. Definitely willing to concede that I'm not as experienced in this code base. In a vacuum, however, that first onChange
call really looks superfluous to me.
So if at all, I'd cleanup the onChange on line 140 instead. Simply because there the check for comparation is more strict. Need to investigate why it is there. I am sure it was not by mistake.
So if at all, I'd cleanup the onChange on line 140 instead. Simply because there the check for comparation is more strict.
I don't think you'd want to get rid of that onChange. If you did, then the consumer would never be alerted to values
updated via setState
. Without that update, the consumer wouldn't pass in updated values
. Basically, this component wouldn't work in it's default state (where methods.addItem
is called in the onClick
in Item.js).
Need to investigate why it is there. I am sure it was not by mistake.
Absolutely! My experience in this component is limited for sure, so I won't discount the possibility that there is some use case that requires that onChange
on L121. But it definitely seems extraneous for any case I've thought through.
Thanks for looking into it!
Hey, it seems that this whole block was added to support callback for drop-down closing events.
This is the relevant commit. https://github.com/sanusart/react-dropdown-select/commit/c5363474f002852a07ae713e8fb335270d2dfc78
I'll try to look into why we need two somewhere next week hopefully.
Thank you for taking the time to look into it BTW!
Looking at the commit you referenced, it doesn't look like that block was added there. It just looks like it was reformatted, likely due to prettier.
You totally right. I am on the phone so easy to miss. I'll try to trace the real reason. And it's commit when with bigger screen.
@RookY2K I'll tell you where my doubt comes from. compareValuesFunc gives ability for overwrite comparator via external prop provided to the component. This flow is different from regular prevProps/currentProps bellow it.
This flow is different from regular prevProps/currentProps bellow it.
There isn't a prevProps/currentProps comparison of values
below the pertinent section.
While the ability of the consumer to override the comparison function does add complexity, it's still the same process. If the current values
are different than the previous values
, based on that comparator function, then it will set the new values
prop to state, trigger an onChange
, then have that onChange
triggered again because the values
state would have changed.
Perhaps the right solution is to add another condition to the state comparison? Something like this:
if (prevState.values !== this.state.values &&
!this.props.compareValuesFunc(this.state.values, this props.values) /*new conditional*/) {
this.props.onChange(this.state.values);
this.updateSelectBounds();
}
This would stop the unnecessary onChange
call without removing it from the props comparison section.
The idea being that if the state version of values
is equivalent to the prop version, then there is no real reason to notify the consumer.
Combined condition would not change a thing imho, moreover degrade readability.
The problem with overriding callback is that it is on its on flow. Same flow of the resetting the value from outside.
In both cases onChange need to be called to notify the listeners. I just checked, first, more strict condition does not being met on inner componnet changes of the value.
On Sat, Dec 17, 2022, 8:53 PM RookY2K @.***> wrote:
Perhaps the right solution is to add another condition to the state comparison? Something like this:
if (prevState.values !== this.state.values && !this.props.compareValuesFunc(this.state.values, this props.values) // new conditional) { this.props.onChange(this.state.values); this.updateSelectBounds(); }
This would stop the unnecessary onChange call without removing it from the props comparison section.
— Reply to this email directly, view it on GitHub https://github.com/sanusart/react-dropdown-select/pull/259#issuecomment-1356384801, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACBLRXOKVOITHSXTTKBMODWNYD4LANCNFSM6AAAAAATBQU4KY . You are receiving this because you commented.Message ID: @.***>
@RookY2K are you dealing with some particular case where some extra rerenderers happening?
I have been, but I've made both changes I proposed locally and it hasn't made a difference. I'm going to close this PR. Thanks for bearing with me. I've got a couple other avenues to explore.
Thank you!
As far as I can tell, this
onChange
call is extraneous.onChange
will be called again in the next update cycle whenprevState.values
andthis.state.values
are not equal (src/index.js#L139-L142).