Selection constructor gives issue when used in a hook with generic type

[schemburkar]

I'm trying to create a hook for using Selections provided by DetailsList.

Short summary of the issue:

export function useSelections3<T>() {
    
    type Test:{};

    const selectionComponent1= new Selection<T>({}); // This line gives error : Argument of type '[{}]' is not assignable to parameter of type 'T extends IObjectWithKey ? [] | [ISelectionOptions<T>] : [ISelectionOptionsWithRequiredGetKey<T>]'.ts(2345)

    const selectionComponent2= new Selection<Test>({});
    
}

Actual Details

I expect something like below to work, but is does not

export function useSelections<T>(): [ISelection<T>] {
    
    const selectionComponent: ISelection<T> = new Selection<T>({
        onSelectionChanged: () => {
            //Do something here
        }
    });
    return [selectionComponent];
}

It would give the following error:

If I use the non generic version or local type, it works for some reason.

export function useSelections1<T>(): [ISelection] {
    
    const selectionComponent: ISelection = new Selection({
        onSelectionChanged: () => {
            //Do something here
        }
    });
    return [selectionComponent];
}


type Test={}
export function useSelections2(): [ISelection<Test>] {
    
    const selectionComponent: ISelection<Test> = new Selection<Test>({
        onSelectionChanged: () => {
            //Do something here
        }
    });
    return [selectionComponent];
}

I would expect the first version to work. Does the constructor on Selection need a explicit type rather than a ternary operator ?

Component: Selection
Version: 7.110.5

[ecraig12345]

I think it should work if you do T extends IObjectWithKey.

The types for the constructor should be correct--they're just unfortunately rather hard to read because we couldn't find a better way to express the constraints we wanted. It's explained in the comments on the constructor (copied below) but basically the types are that way to ensure that you either provide items with a key, or provide options with a getKey function.

  /**
   * Create a new Selection. If `TItem` does not have a `key` property, you must provide an options
   * object with a `getKey` implementation. Providing options is optional otherwise.
   * (At most one `options` object is accepted.)
   */
  constructor(
    ...options: TItem extends IObjectWithKey // If the item type has a built-in key...
      ? [] | [ISelectionOptions<TItem>] // Then the arguments can be empty or have the options without `getKey`
      : [ISelectionOptionsWithRequiredGetKey<TItem>] // Otherwise, arguments require options with `getKey`.
  )

[schemburkar]

@ecraig12345 I've tried setting T​ ​extends​ ​IObjectWithKey on the hook. Error remains same, hence I was confused.

[paulgildea]

@ecraig12345 Pinging on this issue 😄

[ecraig12345]

I messed with this some too and confirmed that it continues to give an error even if T is explicitly declared as T extends IObjectWithKey--and I have no idea why. It almost seems as if having 2 layers of generics messes up the type inference somehow? @ThomasMichon any ideas?

[ecraig12345]

This is the code I used (playground link):

export function useSelections<T extends IObjectWithKey>() {
  type Test = {};

  // errors for both of these
  const selection1 = new Selection<T>({});
  const options2 = {} as ISelectionOptionsWithRequiredGetKey<T>;
  const selection2 = new Selection<T>(options2);
  const options3 = {} as ISelectionOptions;
  const selection3 = new Selection<T>(options3);

  // works
  const selection4 = new Selection<Test>({});
}

[ecraig12345]

I talked about this with Thomas and I'm not sure how much we'll be able to do here.

At first we thought the fact that IObjectWithKey.key is optional was the problem (it should probably be required, but we can't change that outside a major version), but even making it required doesn't fix the problem.

What we really want to express is something like this for the type of the Selection class constructor, except TS doesn't currently allow type parameters inline for the constructor.

class Selection<TItem = IObjectWithKey> {
  // NOT SUPPORTED BY TS
  constructor<T extends Required<IObjectWithKey>>(options?: ISelectionOptions<T>): Selection<T>;
  constructor<T>(options: ISelectionOptionsWithRequiredGetKey<T>): Selection<T>;
  // + implementation...
}

One workaround (which would have the desired results for enforcing types) is something like this:

export class _Selection<TItem = IObjectWithKey> {
  /* ... current Selection implementation ... */
}

interface ISelectionConstructor {
  new <T extends Required<IObjectWithKey>>(options?: ISelectionOptions<T>): _Selection<T>;
  new <T>(options: ISelectionOptionsWithRequiredGetKey<T>): _Selection<T>;
}

export const Selection = _Selection as ISelectionConstructor;
export type Selection = typeof _Selection;

However this has some major downsides:

• It's generally convoluted
• It would be hard to get the documentation to show up in a clear/helpful way on our documentation site
• I'm not entirely sure what the implications (if any) would be for consumers.

[schemburkar]

Thanks @ecraig12345 and @ThomasMichon for the analysis. Appreciated!

I agree with you that the solution is not straightforward and that is fine. One thing to note is that the syntax of Selection<T>is also broken anyways for generic types.

A suggestion: can we have a simpler, clearer solution if we go for a breaking change targeted for a major release?

I would rather have this fixed in the best way than a complex fix or open issue.

[ecraig12345]

Unfortunately I'm not sure a clean solution exists with current TS features, since what we need for a clean solution is type parameter inference in the constructor (I suspect someone has filed an issue with TS about this but am having trouble finding it).

Not sure what you mean by this?

One thing to note is that the syntax of Selection<T>is also broken anyways for generic types.

[schemburkar]

@ecraig12345 sorry if I wasn't that clear. What I meant was anyone wanting to use the Selection api in generic form, like your example gives errors

Generic usage:

export​ ​function​ ​useSelections​<​T​ ​extends​ ​IObjectWithKey​>​(​)​ ​{​
  ​// errors 
  ​const​ ​selection1​ ​=​ ​new​ ​Selection​<​T​>​(​{​}​)​;​
​}

Even if we don't have a clean solution, something like above needs to work.

So I think the Selection code base can be refactored so that it's usage works, even if it could mean relaxing certain annotations on the constructor. Something like:

constructor​(​
    ...​options​: ​ ​{} | ​​ISelectionOptions​<​TItem​>​ ​|
      ISelectionOptionsWithRequiredGetKey​<​TItem​>​ | undefined 
  ​)

However any of this would need a breaking change in a major version. I'm open to further discussions.

[msft-github-bot]

@ecraig12345

Gentle ping that this issue needs attention.

[nlips]

In my opinion the typescript compiler has a bug concerning constraints on type parameters.

I want to be able to define a generic hook associated to a type that does not implement IObjectWithKey :

import * as UI from 'office-ui-fabric-react';

interface IObjectWithId {
    readonly Id: string;
}

export function useSingleSelection<TItem extends IObjectWithId>() {
    const selectionOptions: UI.ISelectionOptionsWithRequiredGetKey<TItem> = {
        getKey(item: TItem, index?: number): string | number {
            return item.Id;
        },
        selectionMode: UI.SelectionMode.single
    };
    const selection = new UI.Selection<TItem>(selectionOptions);
    ...
}

selectionOptions is not accepted in the UI.Selection constructor

Argument of type '[ISelectionOptionsWithRequiredGetKey]' is not assignable to parameter of type 'TItem extends IObjectWithKey ? [] | [ISelectionOptions] : [ISelectionOptionsWithRequiredGetKey]'. TS2345

The workaround is to use any instead of TItem in selectionOptions declaration and cast the selection object.

export function useSingleSelection<TItem extends IObjectWithId>() {
    const selectionOptions: UI.ISelectionOptionsWithRequiredGetKey<any> = {
        getKey(item: TItem, index?: number): string | number {
            return item.Id;
        },
        selectionMode: UI.SelectionMode.single
    };
    const selection = new UI.Selection<any>(selectionOptions) as UI.ISelection<TItem>;
    ...
}

[msft-fluent-ui-bot]

Because this issue has not had activity for over 150 days, we're automatically closing it for house-keeping purposes.

Still require assistance? Please, create a new issue with up-to date details.

[msft-fluent-ui-bot]

Because this issue has not had activity for over 180 days, we're automatically closing it for house-keeping purposes.

Still require assistance? Please, create a new issue with up-to date details.