[Autocomplete] Grouping logic is broken

[liangchunn]

• The issue is present in the latest release.
• I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

Given a set of data and the groupBy functor:

const data = [
  {
    group: 1,
    value: "A",
    intrinsicProperty: "#"
  },
  {
    group: 2,
    value: "B",
    intrinsicProperty: "#"
  },
  {
    group: 2,
    value: "C",
    intrinsicProperty: "#"
  },
  {
    group: 1,
    value: "D",
    intrinsicProperty: "#"
  }
];


const groupBy = datum => datum.group

The logic as described here produces a grouping of:

[
  {
    "key": 1,
    "index": 0,
    "options": [
      {
        "group": 1,
        "value": "A"
      }
    ]
  },
  {
    "key": 2,
    "index": 1,
    "options": [
      {
        "group": 2,
        "value": "B"
      },
      {
        "group": 2,
        "value": "C"
      }
    ]
  },
  {
    "key": 1,
    "index": 3,
    "options": [
      {
        "group": 1,
        "value": "D"
      }
    ]
  }
]

Which either:

• shows on the Autocomplete suggestions as

 1:
    A
 2:
    B
    C
 1:
    D

• crashes the app with

Warning: Encountered two children with the same key, `1`. Keys should be unique so that components maintain their identity across updates.

Expected Behavior 🤔

I expect that the grouping would look something like this:

 1:
    A
    D
 2:
    B
    C

Steps to Reproduce 🕹

Steps:

1. Go to https://codesandbox.io/s/withered-silence-jcrgd
2. Click on the input box
3. See error

Context 🔦

The logic linked in "Current Behavior" is flawed, particularly this part:

if (acc.length > 0 && acc[acc.length - 1].key === key) {
    acc[acc.length - 1].options.push(option);
} else {
    acc.push({
        key,
        index,
        options: [option],
    });
}

A more contrived example:

const data = [
    { group: 1, value: "A" },
    { group: 2, value: "B" },
    { group: 2, value: "C" },
    { group: 1, value: "D" },
    { group: 3, value: "E" },
    { group: 2, value: "F" },
    { group: 1, value: "G" }
]

const groupBy = datum => datum.group

Running the 'faulty' logic yields the result (which causes buggyness due to duplicate key):

[ { key: 1, index: 0, options: [ [Object] ] },
  { key: 2, index: 1, options: [ [Object], [Object] ] },
  { key: 1, index: 3, options: [ [Object] ] },
  { key: 3, index: 4, options: [ [Object] ] },
  { key: 2, index: 5, options: [ [Object] ] },
  { key: 1, index: 6, options: [ [Object] ] } ]

Replacing the logic with:

if (groupBy) {
  let index = 0
  const indexByKey = {}
  const result = []
  for (const option of filteredOptions) {
    const key = groupBy(option)
    if (indexByKey[key] === undefined) {
      indexByKey[key] = index
      result.push({
        key,
        index,
        options: []
      })
      index++
    }
    result[indexByKey[key]].options.push(option)
  }
  let counter = 0
  for (const option of result) {
    option.index = counter
    counter += option.options.length
  }
  groupedOptions = result
}

yields the correctly grouped options:

[ { key: 1, index: 0, options: [ [Object], [Object], [Object] ] },
  { key: 2, index: 3, options: [ [Object], [Object], [Object] ] },
  { key: 3, index: 6, options: [ [Object] ] } ]

Your Environment 🌎

Tech	Version
Material-UI Core	v4.8.3
Material-UI Lab	v4.0.0-alpha.39
React	v16.11.0
Browser	Chrome
TypeScript	v3.7.2

[oliviertassinari]

I like the proposal. The order seems to be preserved and the performance impact looks OK.

The current logic only groups sequential options, but it should probably group them all. My only concern with your proposed solution is the usage of for (const option of, I'm not sure how it transpiles, maybe use forEach instead?

[liangchunn]

Could be implemented in another way, I was just illustrating the desired result 😄

Looping the array with for would be doable as well, and the perf shouldn’t be impacted that much.

[Janpot]

🙂 Also be careful with this code, it will break on:

const groupBy = () => 'toString'

I'd suggest to use

const indexByKey = Object.create(null)

or a Map

[jeunesseBurce]

@oliviertassinari I am encountering a same problem in grouping. Tried to sort it but what I need to grouped are data with nested children on it. And I wanted it to be grouped by its parent node. Would this be possible using AutoComplete?

data is like as follows:

[{
  name: "1"
  parent: null
      children: [
        {
          name: "1.1"
          parent: "1"
        },
        {
         name: "1.2"
         parent: "1"
       }
      ]
}]

with this kind of data it shows this error:
Material-UI: The options provided combined with the groupBymethod of Autocomplete returns duplicated headers. You can solve the issue by sorting the options with the output ofgroupBy.

Is there a way for me to have no duplicates and group them all by their specified parent?

[oliviertassinari]

@jeunesseBurce Please ask support questions on StackOverflow