[Labs] Propagate event correctly when handling empty list #1256
Conversation
|
Gilad is out of town but @gscshoyru will take a look. Thanks for the contribution! |
|
@k-simons what exactly is the edge case here? What was the bad behavior, before? |
| private handlingEmptyListCorrectly() { | ||
| const emptyFilteredList = this.state.filteredItems.length === 0; | ||
| const activeItemIsUndefined = this.props.activeItem === undefined; | ||
| return emptyFilteredList && activeItemIsUndefined; |
There was a problem hiding this comment.
I think this honestly might make more sense to inline, above, since it's just in one place, and the function name is not particularly helpful. Plus the logic is harder to follow like this, I think.
There was a problem hiding this comment.
@gscshoyru inline into componentDidUpdate ?
There was a problem hiding this comment.
Yeah, in the if statement condition. The fact that I have to demorgan's law on this in my head while going back and forth between two lines makes this somewhat difficult to parse.
|
@k-simons Can you please fill up the description and provide details about the edge case you hit, before/after behaviors, etc? Thanks! |
llorca
changed the title
|
@gscshoyru @llorca apologies, gilad and I had talked about this a little Friday. Updated with context |
llorca
changed the title
|
@gscshoyru fixed |
| @@ -186,7 +186,7 @@ export class QueryList<T> extends React.Component<IQueryListProps<T>, IQueryList | |||
| this.shouldCheckActiveItemInViewport = false; | |||
| } | |||
| // reset active item (in the same step) if it's no longer valid | |||
| if (this.getActiveIndex() < 0) { | |||
| if (this.getActiveIndex() < 0 && (this.state.filteredItems.length !== 0 || this.props.activeItem !== undefined)) { | |||
There was a problem hiding this comment.
Much easier to understand.
It might be prudent to add a bit more to the above comment explaining what this is doing (essentially, don't fire the event if there's nothing to pick and the active item is already undefined) but it's probably fine either way.
|
@gscshoyru updated comment. Should be good to merge |
|
fixed lint 🙃 |
| if (this.getActiveIndex() < 0) { | ||
| // Also don't fire the event if the active item is already undefined and there is nothing to pick | ||
| if (this.getActiveIndex() < 0 && | ||
| (this.state.filteredItems.length !== 0 || this.props.activeItem !== undefined)) { |
There was a problem hiding this comment.
Should we expand this check to account for null values too? (i.e. this.props.activeItem != null)
There was a problem hiding this comment.
@cmslewis sure will work on a unit test. I don't think we should add null. Mostly because if this.state.filteredItems is empty, then this.state.filteredItems[0] is undefined. In that case there actually is a difference to props.selectedItem and the event (null vs undefined)
|
@cmslewis tests added |
|
Thanks, @k-simons! |
Fixes #0000
Checklist
Changes proposed in this pull request:
Right now if you have a class that instantiates a raw queryList and passes in an empty filtered list and undefined as the active state, then on
componentDidUpdateit will call update and pass an active item of undefined. If you have a parent class that listens toonActiveItemChangeand then sets its state, the parent class will be re-rendered. Then the queryList will will be re-rendered, and so on and so forth. This will cause an infinite re-rendering loop and a stackover-flow error. You can emulate this behavior if you remove @pure-render from select.tsx and remove:so the query list is always rendered.
Then if you filter to an empty list in select.tsx, you will get a stack-overflow error
Reviewers should focus on:
Screenshot
@giladgray This fixes an edge case of propagating an incorrect change event