Downshift: More flexibility, less API?

[kentcdodds]

What do folks think about doing something like this:

const ui = (
  <Autocomplete getValue={a => a.title}>
    {({getInputProps, getItemProps, isOpen}) =>
      <div>
        <input
          {...getInputProps({
            placeholder: 'hello world',
            ref: node => (this.myOwnReference = node),
          })}
        />
        {isOpen
          ? <div>
              {items.map(i =>
                <article {...getItemProps({key: i.id})}>
                  {i.articleDetails}
                </article>
              )}
            </div>
          : null}
      </div>}
  </Autocomplete>
)

The entire API consists of a handful of props you pass to Autocomplete and a handful of arguments you receive in that callback. No need for props like component because you render the component yourself and you call a function to get the props to apply to the component so it hooks up properly with the autocomplete. In addition, there's no need of using the context API anymore which make the implementation a little nicer.

I'm actually pretty excited by this idea (originally inspired by @jaredly a week ago or so). I tried it and it didn't work very well, but now I think I know how to make it work and I think it'll be great. Minimizing the API and enhance the flexibility/simplicity of the thing. I'm probably going to try to implement this as soon as I can. Just wanted to hear your thoughts :smile:

[kentcdodds]

I've started work on this and gotten a little way of the way there. Would love feedback: https://codesandbox.io/s/mQR4ZlyVE

Even if we don't go with this approach, I really like the way that I changed how the a11y status message works. Give that a look :smile:

[geoffdavis92]

@kentcdodds I like this pattern a lot more. Perhaps it's because I hadn't really used something similar to the Autocompletely pattern before, but I found it a bit difficult to understand OOB, and perhaps a little "magical".

This is a lot easier to wrap my head around. 👍

[souporserious]

This looks great to me. I love the flexibility it seems to offer 😍 And I love that it's nothing special and just functions. Going to play around with it in the morning!

[kentcdodds]

Just a quick update. I tried to make getItemProps work and it does, but we're not going to do it for two reasons:

1. It requires that we return a ref prop so we can scroll the node into view. This is fine unless you're applying these props to a composite component (custom component). In that case, the ref is set to the instance of the composite component and not a DOM node that we can scroll to. This is a real drag, but this alone is reason enough to not do it. The API would be confusing and non-ergonomic (we'd have to require people forward the prop correctly (using a custom innerRef) or only render native DOM components.
2. Performance is not very good and there's not a really good way to make it much better.

That said, we should be able to do away with several other components which may be worth the effort. Please do play around with this!

[kentcdodds]

Oh, here's my try at getItemProps if you're curious: https://codesandbox.io/s/KZllZgEOR

[kentcdodds]

I've managed to widdle it down to only the Autocomplete and Autocomplete.Input components. Pretty interesting. Still evaluating this... https://codesandbox.io/s/mQR4ZlyVE

[souporserious]

Did you get any further on this? I was wondering if the getInputProps needed to be a function and if it could just be inputProps similar to here in react-autosuggest. I think this could work nice since you can just intercept inputProps.ref and pass it to something like an innerRef if you need to.

[kentcdodds]

Here's two options we have

This is basically what we have now (except I've removed Autocomplete.Controller and made the Autocomplete an Autocomplete.Controller instead):

const ui = (
  <Autocomplete getValue={i => i.name} onChange={item => alert(item.name)}>
    {({
      value,
      highlightedIndex,
      isOpen,
      toggleMenu,
      clearSelection,
      selectedItem
    }) =>
      <div>
        <Autocomplete.Input
          isOpen={isOpen}
          placeholder="Find a Star Wars character"
        />
        {selectedItem
          ? <button
              css={{ paddingTop: 4 }}
              onClick={clearSelection}
              aria-label="clear selection"
            >
              <XIcon />
            </button>
          : <Autocomplete.Button
              onClick={toggleMenu}
              aria-label={isOpen ? 'close menu' : 'open menu'}
            >
              <ArrowIcon isOpen={isOpen} />
            </Autocomplete.Button>}
        {!isOpen
          ? null
          : <Autocomplete.Menu>
              {(value
                ? advancedFilter(items, inputValue)
                : items).map((item, index) =>
                <Item
                  value={item}
                  index={index}
                  key={item.id}
                  isActive={highlightedIndex === index}
                  isSelected={selectedItem === item}
                >
                  {getValue(item)}
                </Item>
              )}
            </Autocomplete.Menu>}
      </div>}
  </Autocomplete>
)

The drawback here is that the components will render a div or input as the case may be and if you want to render your own thing you'd have to support a component prop. Also if you want to use a ref prop to get the node, we'd have to expose an innerRef prop. That's more API and quirks for folks to learn.

Here's the API for what I have already in this codesandbox:

const ui = (
  <Autocomplete getValue={i => i.name} onChange={item => alert(item.name)}>
    {({
      value,
      highlightedIndex,
      isOpen,
      toggleMenu,
      clearSelection,
      rootRef,
      selectedItem,
      getButtonProps,
      getInputProps
    }) =>
      <div ref={rootRef}>
        <div>
          <input
            {...getInputProps()}
            isOpen={isOpen}
            placeholder="Enter some info"
          />
          {selectedItem
            ? <button
                css={{ paddingTop: 4 }}
                onClick={clearSelection}
                aria-label="clear selection"
              >
                <XIcon />
              </button>
            : <button {...getButtonProps()}>
                <ArrowIcon isOpen={isOpen} />
              </button>}
        </div>
        {isOpen &&
          <div>
            {(value ? advancedFilter(items, value) : items).map((item, index) =>
              <Autocomplete.Item
                value={item}
                index={index}
                key={item.code}
                isActive={highlightedIndex === index}
                isSelected={selectedItem === item}
              >
                {item.name}
              </Autocomplete.Item>
            )}
          </div>}
      </div>}
  </Autocomplete>
)

The drawback here is that we cannot have a getItemProps because of the reasons mentioned above. So it may be a little confusing to have a component for some things and not for others. But the benefit is that we don't have to worry about an innerRef or component property, so the API is a little more straightforward I think...

I'd really appreciate some feedback and thoughts here...

[souporserious]

What was the reason for bad perf? I think trying to do what I mentioned above might be a pretty nice path. I'll try to fork what you've done and see if I can make it work. I think it should be expected for refs to be wonky and we can mention that in the docs. Even the way we do it now with the component prop doesn't really work because you run into ref issues if your custom component doesn't return a DOM element.

[kentcdodds]

We can't do the inputProps thing without a wonky API anytime someone wants to add an onChange or onKeyDown listener. In our function we compose those together. If we just used an object, then folks would have to compose them themselves. That wouldn't be nice.

I'm not certain what the bad perf problem was. You can investigate it a bit if you like.

[souporserious]

After looking I see that getInputProps is trying to compose those handlers together. I think this should be on the user in my opinion since it's more expected whether you want to override before or after the event. Plus, you only run into that if you need that additional functionality. I think as long as it is mentioned in the docs it would be fine.

[souporserious]

Ah just saw your response 😇 what if we just expose compose or some other util and let them do it when they need to?

[kentcdodds]

But what if we want to add an onKeyUp handler for implementation reasons in the future? That'd be a breaking change for anyone currently using an onKeyUp prop. I really don't want to expose those implementation details to folks. I think that's a major footgun to require people to compose the onClick handler themselves.

[souporserious]

Ahh yeah that's true 😕 good thinking. Maybe what we have currently is good? 🤷‍♂️

[kentcdodds]

Yeah, that's what I'm thinking. Despite the innerRef and the component props which I'm not a big fan of, I think that this is the most simple...

[souporserious]

Ahhh this stinks 💩 I've tried so many different things to get around this haha. I wish we could just use findDOMNode 💀

[kentcdodds]

Why can't we just use findDOMNode?

[souporserious]

It's all here 😞

[kentcdodds]

Gotcha, that's fine. I think we'll just leave things as they are for now.

[kentcdodds]

So... Here's the thing. When we render it ourselves, we make and rely on assumptions. For example: We apply a ref prop on whatever we render for the input. But if the user wants to render a custom component, that ref will be assigned to the instance of their component rather than the input that they render. This will break things.

So with that, I think I'm going to go with this. I don't really like that we render anything. I want the user to choose everything and we'll just make it work. That's the API I'm committed to. We'll make it work, then make it work well, then make it fast.

[kentcdodds]

I'm thinking that findDOMNode will be unavoidable here... And honestly I wonder if we can make it lazy for the items. That would probably speed things up...

[kentcdodds]

We could also apply data- attributes on the items and query for them when they're highlighted or something... 🤔

I don't mind the implementation being a tiny bit messy if it means exposing an API that's really clean, flexible, and simple.

[souporserious]

I think findDOMNode would be great! The only thing that worries me about it is that it might go away in the future.

[souporserious]

Oh I like the data attribute idea! We've done that in the past at my work and it's worked well.

[no1melman]

I used that data- to create a global click handler that hooked back into a list of items - works really well, doesn't seem dirty either. Although I did end up recursively going up parentElement till I found one with the correct class....

[kentcdodds]

Problem still remains though... If you're rendering a custom component, you'll be required to forward all props to the underlying dom element. But I don't think that can be helped with any of our stuff anyway. At some point you need to send the props we send you to the right place.

[kentcdodds]

Here's a WIP that I'm pretty happy with: https://github.com/paypal/react-autocompletely/pull/68

Please give it a look!