Make getKey and selection props mutually exclusive

[okonech]

Current Behavior

DetailsList allows passing the "getKey" prop along with a "selection" prop. This is confusing to users, since the "getKey" prop function will never be called. Instead, the Selection object passed in the "selection" prop either explicitly declares a getKey function, or uses the default getKey implementation (item) => item.key.
The function passed in the "getKey" prop is only used when a Selection object is not passed in the "selection" prop. In this case, DetailsListBase creates a new internally passed Selection object which is passed the function from "getKey" prop.

User Impact

Items may not receive proper selection when loading new data. Items may not retain checked status when the Detailslist is sorted

Workaround

Do not pass the getKey function if the Selection object is passed. Users must instead define the getKey function on the Selection object.

New Behavior

DetailsList will warn uses in non-production environments when "getKey" and "selection" props are passed simultaneously. Making "getKey" and "selection" mutually exclusive DetailsList properties prevents users from making this mistake.

Related Issue(s)

(#23614)

Fixes #

[fabricteam]

📊 Bundle size report

_{🤖 This report was generated against a4e6775a446d201c641c9d10c455efd7b6e66cba}

[fabricteam]

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations	Status
BaseButton	mount	802	805	5000
Breadcrumb	mount	2222	2246	1000
Checkbox	mount	2132	2140	5000
CheckboxBase	mount	1865	1895	5000
ChoiceGroup	mount	3733	3745	5000
ComboBox	mount	911	896	1000
CommandBar	mount	8258	8225	1000
ContextualMenu	mount	15753	16572	1000
DefaultButton	mount	981	987	5000
DetailsRow	mount	2901	2906	5000
DetailsRowFast	mount	2906	2905	5000
DetailsRowNoStyles	mount	2643	2617	5000
Dialog	mount	3464	3471	1000
DocumentCardTitle	mount	315	322	1000
Dropdown	mount	2545	2561	5000
FocusTrapZone	mount	1507	1513	5000
FocusZone	mount	1475	1454	5000
GroupedList	mount	51034	59206	2
GroupedList	virtual-rerender	22068	24523	2
GroupedList	virtual-rerender-with-unmount	75685	74878	2
GroupedListV2	mount	303	312	2
GroupedListV2	virtual-rerender	291	290	2
GroupedListV2	virtual-rerender-with-unmount	314	317	2
IconButton	mount	1403	1423	5000
Label	mount	460	460	5000
Layer	mount	3585	3587	5000
Link	mount	531	530	5000
MenuButton	mount	1233	1232	5000
MessageBar	mount	27509	27526	5000
Nav	mount	2547	2532	1000
OverflowSet	mount	1008	1016	5000
Panel	mount	2240	2251	1000
Persona	mount	980	980	1000
Pivot	mount	1165	1192	1000
PrimaryButton	mount	1093	1098	5000
Rating	mount	5738	5776	5000
SearchBox	mount	1186	1164	5000
Shimmer	mount	2411	2369	5000
Slider	mount	1713	1737	5000
SpinButton	mount	3749	3701	5000
Spinner	mount	522	528	5000
SplitButton	mount	2463	2460	5000
Stack	mount	546	533	5000
StackWithIntrinsicChildren	mount	1146	1157	5000
StackWithTextChildren	mount	3223	3213	5000
SwatchColorPicker	mount	8277	8143	5000
TagPicker	mount	2004	2004	5000
Text	mount	500	499	5000
TextField	mount	1156	1203	5000
ThemeProvider	mount	1123	1146	5000
ThemeProvider	virtual-rerender	772	798	5000
ThemeProvider	virtual-rerender-with-unmount	1718	1731	5000
Toggle	mount	803	804	5000
buttonNative	mount	280	290	5000

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 58d2f32:

Sandbox	Source
@fluentui/react 8 starter	Configuration
@fluentui/react-components 9 starter	Configuration

[size-auditor]

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: a4e6775a446d201c641c9d10c455efd7b6e66cba (build)

[okonech]

@spmonahan Could you please take a look at this?

[spmonahan]

@okonech Thanks for the PR and sorry it's taken me a bit to look at it.

I agree that the existing behavior is confusing but this PR is a breaking API change so we cannot accept it as-is.

In reviewing your PR I looked into Selection's implementation came across this comment in Selection.ts:

/** Custom logic to generate item keys. Required if `TItem` does not have a `key` property. */
  getKey?: (item: TItem, index?: number) => string | number;

Of course this is far from ideal because not only do you have to read the Fluent source code to be aware of it, but it's also counterintuitive that you'd need to provide getKey() in two places. At a minimum we can do better by demonstrating the correct usage for Selection and getKey in the docs.

Perhaps there is also an opportunity to change the way selection is initialized in DetailsList instead of changing the API?

What do you think?

cc @behowell

[spmonahan]

This change is a breaking API change. Putting a block on merging it while other options are discussed.

[okonech]

This gets a little wonky, but the simplest non-breaking change would be a new method for the Selection object: setGetKey(getKey: (item: TItem, index?: number) => string | number) : void. Then in the referenced code, we would update the passed selection object with the passed getKey implementation.

It would be cleaner to instead spread the props of the Selection object passed to DetailsList and create a new Selection object (plus the DetailsList's implementation of getKey if a getKey was not provided)

this._selection = props.selection || new Selection({...[spread props.selection values], ...(props.selection.getKey() && { getKey: props.getKey }) });

but that might break use cases where the users rely on their manually created Selection object's reference being passed in some way. What do you think?

[okonech]

@spmonahan do you have any thoughts?

[okonech]

@spmonahan I've made the change to revert the breaking types change. The PR is now limited to warning users when they use the selection and getKey properties as they are intended to be mutually exclusive. This also fixes the recommended storybook example to follow suit.

[okonech]

@ThomasMichon could you please take a look at this?

[okonech]

@ThomasMichon ping :)

[okonech]

@ThomasMichon could you please review?

[fabricteam]

🕵 fluentuiv8 No visual regressions between this PR and main

[khmakoto]

/azp run Fluent UI React - PR and CI

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[msft-fluent-ui-bot]

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes
• Official NPM Release