maxResults prop changes aren't immediately honored

[FilmCoder]

Steps to reproduce

  1. Click the "one" button
  2. Click the typeahead input
  3. Click the "states" button
  4. Click the typeahead input again, notice only one US State shows

https://codesandbox.io/s/rbt-floating-labels-forked-bd8x8n?file=/src/index.js

Expected Behavior

If themaxResults prop is changed, the typeahead input should honor the new maxResults, not the old. For instance, right now if you pass an options array of 1 length along with maxResults=1, click on the input, then if you programmatically change the options and maxResults props to something else, like a longer options array with maxResults=7, then when clicking on the input it only shows 1 suggestion.

It seems this is a bug with derived state. I hate derived state, tricky and bug-prone but sometimes necessary. It looks like shownResults is added to state initially in getInitialState: https://github.com/ericgio/react-bootstrap-typeahead/blob/main/src/core/Typeahead.tsx#L244

But then only updated in certain circumstances, such as when the input changes in in _handleInputChange in Typeaheadmanager: https://github.com/ericgio/react-bootstrap-typeahead/blob/main/src/core/Typeahead.tsx#L244

I think the best way to deal with this is update state in getDerivedStateFromProps, not during input changes. That way state updates immediately on prop change, instead of only on input change.

[ericgio]

Hey @FilmCoder, thanks for filing this issue, and for the detailed repro instructions and sandbox. I can reproduce the behavior you describe and agree this is a bug. That said, I'm curious about the use case for changing the value of maxResults; can you say more about why you'd need this?

One workaround is to update the the Typeahead's React key when updating maxResults, which will re-mount a new instance with the new value:

<Typeahead
  ...
  options={options}
  key={options.length}
  maxResults={options.length}
/>

[FilmCoder]

@ericgio Good tip! So we were actually switching around the maxResults prop unnecessarily, and my hotfix was to just stop doing that. But to answer your question I'll explain the context in which it happened.

I work as US News & World report and we use the Typeahead inputs for a B2B product (Academic Insights) which colleges use to run analytics on each other. We have a variety of datasets (graduate schools, medical, law, engineering, undergrad, etc...) and allow the user to switch between them. When the dataset is switched we don't refresh the page, we instead pass new props to a variety of components (some of which are Typeahead inputs). This makes sense because we have a ton of datasets but the layout of the page is the same across all of them.

We were passing options along with a maxResults equal to options.length. So when switching datasets the maxResults props would change. But, that's totally unnecessary, setting maxResults to match the length of the suggestions array would have no effect on anything and I issued a fix to our codebase to stop doing that.

To be honest I can't really think of a good scenario in which you WOULD be switching up maxResults, because doing so would change the layout/presentation of your page.

[ericgio]

@FilmCoder thanks for the additional context, that's helpful. Glad you were able to address the issue, and I appreciate the bug report. Given that I can't come up with any cases where you'd want to update the maxResults prop, I'm not sure it's worth fixing this particular issue. Using derived state tends to cause as many issues as it solves in my experience. I may just rename the prop to something like defaultMaxResults or something to indicate that it's set on the initial mount only (or maybe just update the documentation to that effect).

[FilmCoder]

@ericgio Hehe that sounds good to me. Wait you're saying you don't want to waste every waking moment of your life fixing the most obscure bugs nobody will ever encounter, filed by strangers on the internet? Are you sure you're cut out to be a coder?