Console errors when using 'toggleMenu' action prop

[ikovic]

• downshift version: 1.3.1
• node version: 8.2.1
• npm (or yarn) version: yarn 0.27.5

Relevant code or config

export default class LanguageDropdown extends Component {
  itemToString = item => item.name;

  toggleDropdown = () => this.setState(state => !state.isOpen);

  render() {
    return (
      <Downshift itemToString={this.itemToString}>
        {({ toggleMenu, isOpen, ...rest }) =>
          <div className={s.inputContainer}>
            <Title onClick={toggleMenu} />
            {isOpen ? <Menu {...{ items: languages, ...rest }} /> : null}
          </div>}
      </Downshift>
    );
  }

What you did:

I've tried using the toggleMenu action and isOpen props to control the visibility of a dropdown menu.

What happened:

isOpen value changes as expected, but the console shows a bunch of these errors:

Problem description:

Console gets a couple of above errors on each execution of toggleMenu action.

Suggested solution:

I could easily get around this by managing the state of isOpen on my own, but it would be better if I could use the downshift actions for that simple use case.

[tansongyang]

I'm seeing the same thing. I made a minimal reproduction and I get the same errors.

[kentcdodds]

Hi! Yeah, so the problem is that onClick handlers are invoked with an event and toggleMenu accepts an argument called otherStateToSet. So if it's passed an event, then downshift tries to set state with the event.

So the solution is simple: onClick={() => toggleMenu()}...

You know... I think we could make this nicer for folks. In toggleMenu, we could check if otherStateToSet is an event, and if it is, then we don't spread it on the state we return in our internalSetState callback. This would be tested here.

Who wants to do this?

[tansongyang]

@kentcdodds I've got time to do this today.

[tansongyang]

A couple of thoughts occurred to me:

1. If we do this in toggleMenu, would we also want to do it in the other places that spread otherStateToSet?
2. If we do, perhaps it makes sense to add a helper function in utils.
3. Instead of testing whether the object is an event, would it be better to follow a "whitelist" policy? That is, no matter what properties the state object has, we only pick out the known safe values, which I believe are highlightedIndex, inputValue, isOpen, and selectedItem, right? That solves this problem, and it potentially solves future ones.

[kentcdodds]

I'm glad you asked! Yes, we should probably do this for all the actions which would commonly be used in this way. (openMenu and closeMenu come to mind)

[tansongyang]

Wow, you respond quickly! I actually just edited my second comment. What do you think of the new points?

[kentcdodds]

Yes, I like your item 3 idea. To avoid the issues though, we'll need to pluck those values off of the given object at the first of the function rather than in the callback. I'm thinking something as simple as:

  toggleMenu = (otherStateToSet = {}, cb) => {
    const {highlightedIndex, inputValue, isOpen, selectedItem, type} = otherStateToSet
    this.internalSetState(
      ({isOpen}) => {
        return {isOpen: typeof isOpen === undefined ? isOpen : !isOpen, highlightedIndex, inputValue, selectedItem, type}
      },
      () => {
        const {isOpen} = this.getState()
        if (isOpen) {
          this.highlightDefaultIndex()
        }
        cbToCb(cb)()
      },
    )
  }

We wouldn't really need a util for this I think... But maybe.

[tansongyang]

Also, you mention closeMenu and openMenu, but they don't accept otherStateToSet. Should they?

[kentcdodds]

Haha, I thought they did, but looks like they don't. We're probably good to leave them as-is

[tansongyang]

Sounds great. I'll PR when I'm finished.