Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

List: Parameterize generic type for item with any as default #8465

Merged
merged 10 commits into from
Mar 26, 2019

Conversation

KevinTCoughlin
Copy link
Member

@KevinTCoughlin KevinTCoughlin commented Mar 25, 2019

Pull request checklist

  • Addresses an existing issue: Fixes #0000
  • Include a change request file using $ npm run change

Description of changes

I've found working with List and components that wrap it difficult as an end-user. This is partially because the backing items Array is typed as any[] and therefore type-safety is lost for TypeScript developers.

Last week on Shield, I watched a developer run into problems during a debug session as a result of this lack of type safety and thought it was worth fixing IFF backwards-compatibility could be maintained.

To reduce this barrier, I'm proposing that we parameterize the generic type for List's items as T which defaults to any.

This should maintain backwards compatibility as item and items will continue to be typed as any if a parameterized type is omitted. If the type is provided, then the intellisense and other goodness should then be provided.

Note: As part of our next major release we should look to remove the need for any as the default type.

IDE Output

Here is a brief comparison of Code's intellisense before and after this change for the code snippet below:

Usage:

export interface IListBasicExampleProps {
  items: IExampleItem[];
}

<List items={items} onRenderCell={this._onRenderCell} />

Before

onRenderCell prop:

(JSX attribute) onRenderCell?: ((item?: any, index?: number | undefined, isScrolling?: boolean | undefined) => React.ReactNode) | undefined
Method to call when trying to render an item.

items prop:

(JSX attribute) items?: any[] | undefined
Items to render.

List class definition

(alias) class List
import List

After

onRenderCell prop:

(JSX attribute) onRenderCell?: ((item?: IExampleItem | undefined, index?: number | undefined, isScrolling?: boolean | undefined) => React.ReactNode) | undefined
Method to call when trying to render an item.

items prop:

(JSX attribute) items?: IExampleItem[] | undefined
Items to render.

List class definition

(alias) class List<T = any>
import List

Focus areas to test

  • CI should pass
  • API file modifications should maintain backwards compatibility

Related to: #5557

Microsoft Reviewers: Open in CodeFlow

@KevinTCoughlin
Copy link
Member Author

My primary concern here is maintaining backwards-compatibility. I believe this change achieves that. However, if it does not then I am fine with targeting 7.0 branch.

@ecraig12345
Copy link
Member

Agreed that with T = any, you'll probably be safe for compatibility. Someone else correct me if I'm wrong.

@size-auditor
Copy link

size-auditor bot commented Mar 25, 2019

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

Copy link
Contributor

@cliffkoh cliffkoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@KevinTCoughlin KevinTCoughlin merged commit 5ad8b88 into microsoft:master Mar 26, 2019
@KevinTCoughlin KevinTCoughlin deleted the keco/make-list-generic branch March 26, 2019 23:21
@msft-github-bot
Copy link
Contributor

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

Handy links:

@KevinTCoughlin
Copy link
Member Author

For anyone that may land here due to compiler errors, the solution is likely as follows:

If you have previously defined your List's onRenderCell method as:

onRenderCell?: (item: Foo, index?: number, isScrolling?: boolean) => React.ReactNode;

Where item of type Foo is not marked as an optional parameter then you will need to mark it as such as it has been defined in IListProps. The modified signature would look like:

onRenderCell?: (item?: Foo, index?: number, isScrolling?: boolean) => React.ReactNode;

Also, in the body of the function you may want to guard against the case where item is undefined.

This bug was previously masked by the usage of any.

@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants