hopefully fix combobox key issue

[balupton]

was using autocomplete in a project, and kept getting these errors:

index.js:1 Warning: Encountered two children with the same key, `ComboBox3-0`. Keys should be unique so that components maintain their identity across updates. Non-unique keys may cause children to be duplicated and/or omitted — the behavior is unsupported and could change in a future version.
    in ul (created by OptionList)
    in li (created by OptionList)
    in ul (created by OptionList)
    in OptionList (created by ComboBox)
    in div (created by ComboBox)
    in div (created by Scrollable)
    in Scrollable (created by Pane)
    in Pane (created by ComboBox)
    in div (created by PositionedOverlay)
    in div (created by PositionedOverlay)
    in div (created by PositionedOverlay)
    in div (created by PositionedOverlay)
    in PositionedOverlay (created by PopoverOverlay)
    in PopoverOverlay (created by Popover)
    in Portal (created by Popover)
    in div (created by Popover)
    in Popover (created by ComboBox)
    in div (created by ComboBox)
    in ComboBox (created by Autocomplete)
    in Autocomplete (at timezone.tsx:126)
    in TimezonePicker (at add.tsx:154)
    in div (created by Item$5)
    in Item$5 (created by FormLayout)
    in div (created by FormLayout)
    in FormLayout (at add.tsx:114)
    in form (created by Form)
    in Form (at add.tsx:113)
    in div (created by Section$4)
    in Section$4 (at add.tsx:112)
    in div (created by Page)
    in div (created by Page)
    in Page (created by WithAppProvider(Page))
    in WithAppProvider(Page) (at page.tsx:25)
    in div (created by Frame)
    in main (created by Frame)
    in div (created by Frame)
    in Frame (created by WithAppProvider(Frame))
    in WithAppProvider(Frame) (at page.tsx:24)
    in MediaQueryProvider (created by AppProvider)
    in div (created by ThemeProvider)
    in ThemeProvider (created by AppProvider)
    in AppProvider (at page.tsx:23)
    in div (at page.tsx:9)
    in Page (at add.tsx:111)
    in AddEventPage (created by App)
    in App
    in Container (created by AppContainer)
    in AppContainer

Which caused unexpected results on the combobox when typing characters, but it would all work fine when backspacing.

Inspecting the autocomplete options that I'm giving the component returns:

(6) [{…}, {…}, {…}, {…}, {…}, {…}]
0: {value: "America/Yakutat", label: "AKST - America/Yakutat (GMT -09:00)", offset: 540, id: "ComboBox3-0"}
1: {value: "America/Resolute", label: "CST - America/Resolute (GMT -06:00)", offset: 360, id: "ComboBox3-1"}
2: {value: "Etc/UTC", label: "UTC - Etc/UTC (GMT +00:00)", offset: 0, id: "ComboBox3-0", active: false}
3: {value: "Africa/Ceuta", label: "CET - Africa/Ceuta (GMT +01:00)", offset: -60, id: "ComboBox3-3"}
4: {value: "Africa/Maputo", label: "CAT - Africa/Maputo (GMT +02:00)", offset: -120, id: "ComboBox3-4"}
5: {value: "Asia/Beirut", label: "EET - Asia/Beirut (GMT +02:00)", offset: -120, id: "ComboBox3-5"}
length: 6
__proto__: Array(0)

As you can see, id properties exist, and one of them is a duplicate. This is odd, as:

1. I do not set the id property at all on the options I provide
2. Even if I do set the id property on the options I provide, the same result occurs

Derefencing the options with the following before I hand them over to autocomplete solves the issue

function dereferenceObjects<T>(items: Array<T>) {
	return items.slice().map(v => Object.assign({}, v))
}

Which then made me suspect that the polaris codebase is just writing to the source option object, instead of creating a new one.

This is a rough fix, I have not tested it.

Thanks to @sumitrai for debugging this with me on our work on Fountain.

You can see the problem in action with:

• commit
• demo

And the fix:

• commit
• demo

Test by typing utc and backspacing it. Notice that backspacing from utc to ut shows the correct result, but going from u to ut does not.

WHY are these changes introduced?

Fixes #0000

WHAT is this pull request doing?

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:

import React from 'react';
import {Page} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      {/* Add the code you want to test in here */}
    </Page>
  );
}

🎩 checklist

• Tested on mobile
• Tested on multiple browsers
• Tested for accessibility
• Updated the component's README.md with documentation changes
• Tophatted documentation changes in the style guide
• For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

[ghost]

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines.

[balupton]

signed the CLA, not sure how to re-run the test

[dleroux]

👋 Thanks for doing this @balupton. Just made a small suggestion and you'll also need to add an entry to the UNRELEASED.md. Hopefully, when you push those up it will clear the CLA issue.

[balupton]

So I can't seem to test this locally because of

> @shopify/polaris@4.10.2 build /Users/balupton/Projects/websites/polaris-react
> node ./scripts/build.js

node_modules/@shopify/react-testing/dist/src/matchers/index.d.ts:6:19 - error TS2428: All declarations of 'Matchers' must have identical type parameters.

6         interface Matchers<R> {
                    ~~~~~~~~

node_modules/@types/jest/index.d.ts:730:15 - error TS2428: All declarations of 'Matchers' must have identical type parameters.

730     interface Matchers<R, T> {
                  ~~~~~~~~

tests/matchers/index.ts:6:15 - error TS2428: All declarations of 'Matchers' must have identical type parameters.

6     interface Matchers<R> {
                ~~~~~~~~


Found 3 errors.

child_process.js:649
    throw err;
    ^

Error: Command failed: /Users/balupton/Projects/websites/polaris-react/node_modules/.bin/tsc --outDir /Users/balupton/Projects/websites/polaris-react/build-intermediate --project /Users/balupton/Projects/websites/polaris-react/scripts/tsconfig.json
    at checkExecSyncError (child_process.js:610:11)
    at execSync (child_process.js:646:15)
    at Object.<anonymous> (/Users/balupton/Projects/websites/polaris-react/scripts/build.js:25:1)
    at Module._compile (internal/modules/cjs/loader.js:1139:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1159:10)
    at Module.load (internal/modules/cjs/loader.js:988:32)
    at Function.Module._load (internal/modules/cjs/loader.js:896:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12)
    at internal/main/run_main_module.js:17:47 {
  status: 2,
  signal: null,
  output: [ null, null, null ],
  pid: 22743,
  stdout: null,
  stderr: null
}
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! @shopify/polaris@4.10.2 build: `node ./scripts/build.js`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the @shopify/polaris@4.10.2 build script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/balupton/.cache/npm/_logs/2020-01-04T11_06_28_661Z-debug.log

[dleroux]

👋 @balupton Have you tried rebasing, I'm not getting these issues on master?

[AndrewMusgrave]

Would you mind adding a small snippet of code reproducing the issue? Thanks! 😄

[AndrewMusgrave]

This code was a little odd and might be obscuring what it's actually doing. Map returns a new array, however, we seem to be mutating the option and not using the result of the map. Ideally, we would have used a forEach, for, for of, etc.

[AndrewMusgrave]

Here we're not mutating the option anymore, or using the result of the map so it'll essentially a no-op.

[balupton]

@AndrewMusgrave would you be able to run with the appropriate amendments. If this is dependent on me, I am doubtful this is going to get fixed. No care if it is my PR that lands. Just wanting the issue fixed.

[alllx]

To use Array.map without use of newly returned array is an anti-pattern: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/map#When_not_to_use_map
You can do this:

options.forEach(
    (
      option: OptionDescriptor | ActionListItemDescriptor,
      optionIndex: number,
    ) => {
      options[optionIndex] = {
        ...option,
        id: `${comboBoxId}-${optionIndex}`,
      };
    },
  );
  return options;

Also, in this fix I would not allow user to set his own id for option as you can see components still relies on his internal id naming in function private selectedOptionId().

[sharkcrayon]

I'm having this issue as well. The results shown in the dropdown are not correct without clicking away from the field and then clicking back again.

What fixes it for me is stripping the Combobox ids on the filtered objects before updating state:

      const filterRegex = new RegExp(value, 'i')
      const resultOptions = initialOptions.filter((option) => {
          return option.label.match(filterRegex)
        }
      )

      // Strip the ids assigned by Autocomplete's Combobox before handing it back to Autocomplete.
      const cleanResultOptions = resultOptions.map(product => {
        return {
          ...product,
          id: '',
        }
      })

    setOptions(cleanResultOptions)

[alllx]

We experienced the same issue on our project, haven't seen this PR, done my own fix here: #2818

[dleroux]

I'm going to close this @balupton as the same fix was shipped here