React-bootstrap-typeahead: `selected` state doesn't always update correctly in v4.0.0-alpha.x

[rsslldnphy]

Version

Issue encountered on versions v4.0.0-alpha.9 and v4.0.0-alpha.8, same code works fine on v3.4.2.

Steps to reproduce

The below component (which filters out custom options before setting the selected value), encounters the issue on the v4 alpha versions - options are displayed regardless of whether they are custom options or otherwise.

import { useState } from 'react';
import { Typeahead } from 'react-bootstrap-typeahead';

const PersonInput = function() {
  const options = [{id: 1, name: 'russell'}, {id: 2, name: 'naadir'}];
  const [ selected, setSelected ] = useState([options[0]]);

  const onChange = function(results) {
    // filter out all custom inputs
    setSelected(results.filter(e => !e.customOption));
  }

  return (
    <Typeahead
      id="personName"
      labelKey="name"
      allowNew={() => true}
      multiple
      options={options}
      minLength={2}
      onChange={onChange}
      selected={selected}
    >
    </Typeahead>
  );
};

Expected Behavior

When selecting an existing option, it should be added as a token to the input; when creating a new, custom option, it should not be added as a token to the input. (The eventual aim of the component I'm building is that an API request would be made to create the token on the backend, and only after this has succeeded would it be added as a token to the input).

Actual Behavior

Both existing options picked from the list and custom options are added as tokens to the input. As a result, the tokens in the input do not match the value of selected which is being passed to the Typeahead as a prop.

Notes

Worth noting that the initial value passed to selected seems to be being used. It's as if selected is being treated as if it were actually defaultSelected.

I figured I was just doing something wrong until I tried downgrading to 3.4.2 and the same code worked exactly as I expected it to.

I'm pretty new to the javascript ecosystem - should I be using the alpha versions as a matter of course or should i be using 3.4.2? yarn add defaulted to installing v4.0.0-alpha.9, which is what i've been using till I encountered this issue - and I was surprised to see an alpha version chosen by default.

[ericgio]

Hey @rsslldnphy, thanks for the detailed report; I can reproduce the issue. Looks like internal state is not being correctly updated when props are passed in in some cases.

should I be using the alpha versions as a matter of course or should i be using 3.4.2?

This library tries to adhere to semantic versioning, so alpha versions are pre-releases and should be considered unstable. They may contain bugs (as you've discovered) or the API may be subject to change. As far as I know, yarn and npm install the latest versions of a package by default (ie: when no version is specified), whether these are stable releases or not. To avoid that, you can just specify the version, which it sounds like you've already figured out.

[daschi]

I'm having what seems like a very similar problem, where the component gets a new selected prop, but it does not seem to get passed down to the TypeaheadManager component. I'd love to help and open a PR for this, would anyone be interested in working together on it (or pointing me in the right direction)? I wrote a failing test, but I am not sure whether it's failing correctly. I see other tests that are using enzyme's wrapper.update() method, which would make this test pass, but I think we shouldn't need to do that when using setProps. Any and all feedback welcome!

...in AsyncTypeahead.test.js... 

import states from '../data'

describe('<AsyncTypeahead>', () => {
  let onSearch, wrapper;

  beforeEach(() => {
    onSearch = jest.fn();
    wrapper = mount(
      <AsyncTypeahead
        delay={0}
        id="async-example"
        isLoading={false}
        minLength={3}
        onChange={noop}
        onSearch={onSearch}
        selected={[]}
        labelKey='name'
      />
    );
  });

  describe('selected prop', () => {
      beforeEach(() => {
        wrapper.setProps({
          multiple: false,
          selected: [states[0]],
        });
      })

    describe('when given a new selected prop', () => {
      it('updates the typeahead input to the new selected prop', () => {

        const alaska = states[1]
        wrapper.setProps({
          selected: [alaska]
        })

        const inputValue = wrapper.find('input').first().props().value
        expect(inputValue).toEqual(states[1].name)
      })
    })
  })

[ericgio]

This has been a pretty challenging bug, both in terms of the behavior as well as properly testing it. Wrt wrapper.update(), I had trouble getting the tests to behave consistently with the browser. The relevant code is located in componentDidUpdate but Enzyme doesn't always seem to trigger that hook as I'd expect. Enzyme also seems to update state in a strictly synchronous manner (as noted in this issue), so I've found it difficult to reproduce browser behaviors in the test environment.

I'd love some help if you're willing to take a crack at this, and happy to point you in the right direction and help debug as best I can. Otherwise, I may need to revert the change that caused this (and related) issues, which would mean going back to using componentWillReceiveProps until I can figure out the right way to update and test the code using non-deprecated lifecycles.

[daschi]

Oh okay--that's good to know about enzyme. If you have any additional insights that would help me get started, and if possible, could link the commit that introduced the change for componentWillReceiveProps, I'd love to take a crack at this!

[ericgio]

Thanks for your willingness to help out, I really appreciate it! Here's a bit of context:

Relevant Commit: https://github.com/ericgio/react-bootstrap-typeahead/commit/ab10829217dd39415573bf305cf8de96ba9e717f
This is the primary change that moved code from componentWillReceiveProps to componentDidMount, though it's not the only one to update the relevant code.

Behavior
The reason for having this code in the first place is to help track and control the state of the typeahead's selections and input value under different circumstances, and to make sure they update correctly based on different user actions. More specifically, when a user hits backspace to delete part of the input value and there is a selection, the selection should be cleared and the input value should update with the new string.

This is relatively straightforward in an uncontrolled component, since the typeahead isn't receiving new selected values when those change, so state can simply be tracked internally without any issues. It becomes complicated when the typeahead is controlled and receiving new selections via props. Here's the scenario I'm talking about, which I imagine to be pretty common:

class MyTypeahead extends React.Component {
  state = {
    selected: [],
  };

  render() {
    return (
      <Typeahead
        ...
        onChange={selected => this.setState({ selected })}
        options={[ ... ]}
        selected={this.state.selected}
      />
    );
  }
};

Given the above, let's say a user makes a selection (eg: "California"). The (partial) typeahead state should now be:

{
  selected: ['California'],
  text: 'California',
}

If the user hits backspace, the state should update to be:

{
  selected: [],
  text: 'Californi',
}

In that case, onChange would be triggered (since the selections changed), causing MyComponent's state to update, which would then be passed back into Typeahead as props. The problem is then how to distinguish between an updated selected prop triggered by the component vs. some other external action. This behavior actually works correctly as of now, but seems to be causing bugs in other scenarios, as the original issue describes and as you seem to have discovered.

Testing
As mentioned, testing needs to cover both the controlled and uncontrolled cases, and depends on the lifecycles being called (and the component updating) as expected. I think this was more straightforward when I was relying on componentWillReceiveProps, whereas with componentDidUpdate there are successive updates that occur in practice that don't seem to occur with Enzyme (hence the use of wrapper.update()). The current solution also relies on state changes being batched and updated asynchronously (which I believe to be a totally reasonable assumption), but as noted above Enzyme treats state changes synchronously.

I know that's a lot, so let me know if you have additional questions. I think just getting another set of eyes on the problem would be a huge help; it's entirely possible that my current approach is misguided and there's a much better or simpler solution that I haven't been thinking of.

[ericgio]

This issue is fixed in v4.0.0-rc.1, but only because I essentially reverted the original change and went back to using componentWillReceiveProps :(

I'll need to spend more time figuring out a long-term solution that doesn't use deprecated lifecycles, but in the meantime it's holding up a bunch of other changes I want to get out.

[ericgio]

I see other tests that are using enzyme's wrapper.update() method, which would make this test pass, but I think we shouldn't need to do that when using setProps.

@daschi: not sure if this is still an issue for you, but based on some additional research, it looks like componentDidUpdate does not get called by enzyme when using mount (It apparently does get called when using shallow, but I haven't actually confirmed that).

Your test is failing because selected is modified in componentDidUpdate, which is not being called by enzyme. Calling wrapper.update() just after setProps seems to be an acceptable solution, and I've seen it suggested on a few related enzyme issues.

[ericgio]

The original issue was fixed for v4.0 using UNSAFE_componentWillReceiveProps and is also fixed in v5.0.0-alpha.1 using componentDidUpdate. Thanks for providing a test case!

[abdulrauf1]

hi
I have use multiple Typeahead, by selecting one's option it causes the selection of all the Typeahead.
Please help me in it.
the code is as under

id={i}
labelKey={option => ${option.name}}
//onChange={setSelectedQuotation}
multiple={true}
onChange={setSelected}
options={allDocs}
placeholder="Choose a Quotation ..."
selected={selected}
/>

where 'i' is the loop iterator.

[abdulrauf1]

Hi!

I have used multiple typeaheads, while selecting one's option it causes the selection of all the typaheads.
please help me in it.

id={i}
labelKey={option => ${option.name}}
//onChange={setSelectedQuotation}
multiple={true}
onChange={setSelected}
options={allDocs}
placeholder="Choose a Quotation ..."
selected={selected}
/>