[Select] Should not crash when an empty array is passed with multiple enabled

[Domino987]

Fixes #29836

📦 https://codesandbox.io/s/mui-issue-latest-forked-4pg71

For the case that an empty list is passed for multiple, a fallback needs to be passed to the reduce.
Otherwise the first element is selected, which is undefined in this case ( empty list ) and it crashes the code.

• I have followed (at least) the PR section of the contributing guide.

References

[mui-pr-bot]

Details of bundle changes

Generated by 🚫 dangerJS against d5d99df

[siriwatknp]

@Domino987 Can you refer to the issue number please, add #issue-number (eg. #29898) to the description of the PR

[Domino987]

I just did. Thanks

[hbjORbj]

Hi, thanks for opening the PR.

The following test fails due to the change of this PR.
Can you make sure it doesn't break any existing test? You can use this command to test: yarn test:unit --grep Select.

1) <Select />
       prop: multiple
         should serialize multiple select display value:

      AssertionError: expected div.MuiSelect-select.MuiSelect-outlined.MuiSelect-multiple.MuiOutlinedInput-input.MuiInputBase-input.emotion-client-render-11u53oe-MuiSelect-select-MuiInputBase-input-MuiOutlinedInput-input[tabindex="0"][role="button"][aria-expanded="false"][aria-haspopup="listbox"] to have text 'Ten, Twenty, Thirty', but the text was ', Ten, Twenty, Thirty'
      + expected - actual

      -, Ten, Twenty, Thirty
      +Ten, Twenty, Thirty
      
      at Context.<anonymous> (packages/mui-material/src/Select/Select.test.js:819:43)
      at processImmediate (internal/timers.js:461:21)

Also, you should add a test to confirm that what this PR is trying to fix (which handles properly when an empty list is passed with multiple enabled) is indeed fixed.

[mnovati]

@Domino987

I don't know the style guide:

displayMultiple.reduce((prev, curr, idx) => idx === 0 ? [curr] : [prev, ', ', curr], '');

[hbjORbj]

@mnovati I think this works :)

@Domino987 It seems like I can't push to your branch. I think I need your permission. remote: Permission to Domino987/material-ui.git denied to hbjORbj. Would you make this change and as I said above add a test?

[Domino987]

Sorry, was afk for a few days due to work. I will add the test

[mnovati]

displayMultiple is an array of nodes and this will stringify them I think and might cause some weird side effects. I'm not familiar with the code base but just something I noticed

[Domino987]

Yes, after a more deep dive, I am now short circuiting the aggregation and also added a test with a zero-width space that gets added for the empty value.
It should work now.

[siriwatknp]

@oliviertassinari Can you have a quick look at the Sandbox (in the description) if this is the expected behavior? It looks okay to me.

[hbjORbj]

@mnovati @Domino987 Thanks for your contribution! :)

@oliviertassinari As can be seen in here, we already render react elements in case of Select with multiple disabled. I think we should keep the consistency so this looks good to me.