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

Allow Table.Column class type to be possible to extend as a generic type in TypeScript #4324

Closed
piotrwitek opened this issue Dec 21, 2016 · 21 comments
Assignees

Comments

@piotrwitek
Copy link

piotrwitek commented Dec 21, 2016

Rationale

In Table class there is a Column Class Type declared as static member, making it not possible to be consumed as generic type.
But Column Class is generic and should infer the type from Table (to be able to leverage type inference for records in render method)

For instance when I try to consume the library in TypeScript I want to extend Table as a generic type like for instance: class UserTable extends Table<User> { };, then I can benefit from record object to be typed.
I would like to do exactly the same for Column but it is not possible, because I'm forced to use static Column member type from Table Class, like this: const { Column } = Table;

Expected usage in TypeScript:

import { User } from './models/user';
import { Table } from 'antd';

class UserTable extends Table<User> { };

const users: User[] = [{...},{...},...];

<UserTable dataSource={users} rowKey={el => el.user_id}> // "el" object should infer User type
  <UserTable.Column title="Name" key="user" render={(text, record) => {
    return <UserItem user={record} />; // "record" object here should also infer User type
  } } />
...
...

Initial Proposal (rejected)

My proposal is to export Column Class as Component so I can import it like Table and extend using generics the same way as with Table: class UserColumn extends Column<User> { };, then I could leverage typing like this:

Updated Proposal:

After consideration of library constrains, my new proposal is to make it possible to extend Column Class as a generic class in the same way as it is currently possible with Table Class, that would be consistent and elegant enough for consumers.

See usage example:

import { Table } from "antd"

// Define a  generic Table 
class MyTable extends Table<IPhoto>{}
// Define the generic Column.
class MyColumn extends Table.Column<IPhoto>{}

Also please check alternative example below: #4324 (comment)

Optimal Solution

Optimal solution would be for the Column to get the generic type out of the extended generic MyTable class type, so the following would work as expected:

<MyTable ... >
  <MyTable.Column ... /> // should have generic type inferred from MyTable automatically
</MyTable>

Please tell me if you are accepting PRs, maybe I can look into this in my free time.

References:

@BANG88
Copy link
Member

BANG88 commented Dec 22, 2016

@piotrwitek Please see microsoft/TypeScript#6395

However, if you want use Generic Table Column you can do it like this:

import { Table } from "antd"
const { Column } = Table;

// Define a  generic Table 
class MyTable extends Table<IPhoto>{}
// Define the generic Column.
class MyColumn extends Column<IPhoto>{}


// Usage
<MyTable dataSource={this.props.data} bordered loading={!this.props.data.length}>
                    <MyColumn
                        title="Thumb"
                        dataIndex="thumbnailUrl"
                        key="thumbnailUrl"
                        render={(thumb, row, index) => {   
                             // Now, row has type IPhoto.                         
                            return <img src={thumb} alt="" height={75}/>
                        } } />
</MyTable>
...
...
...

and export Column is unnecessary. You should use it with Table always.

@BANG88 BANG88 closed this as completed Dec 22, 2016
@benjycui
Copy link
Contributor

Actually, children of Table must be Table.Column..

@piotrwitek
Copy link
Author

@BANG88 have you tested the solution that you propose with success in a real project?
Because I have tried that already before and it didn't work, that's why I have issued this proposal

There seems to be a problem with extending Column, because extended class cease to work.
So example:

import { Table } from "antd"
const { Column } = Table;

// Define a  generic Table 
class MyTable extends Table<IPhoto>{}
// Define the generic Column.
class MyColumn extends Column<IPhoto>{}


// Usage
<MyTable dataSource={this.props.data} bordered loading={!this.props.data.length}>
    <MyColumn ... /> \\ <- this cease to work
    <Column ... /> \\ <- this working fine
</MyTable>

I'm trying to figure out if this is my project setup or is it a problem in your library.

Could you please provide some minimal working setup in some repo that I could check out if you actually tested it?

@benjycui
Copy link
Contributor

Actually, children of Table must be Table.Column..

You cannot use customized component as column now.

@piotrwitek
Copy link
Author

@benjycui @BANG88 @afc163 Thanks for trying to help, but could you please elaborate a little bit more with some alternatives/solutions to the problem, I'm trying to figure out a nice convention on how to use your library in a elegant typed manner.

If I'm not able to extend Column with generic type then I believe your library is not designed correctly from consumer perspective as it should be possible to do so, especially because you library is written in TypeScript.

Could you please tell me what is your solution to this problem, should I open another issue addressing this problem specifically?

@benjycui
Copy link
Contributor

ping @yesmeck

@BANG88
Copy link
Member

BANG88 commented Dec 23, 2016

I don't tested in a real project,Using generic components with typescript please track the links i posted.

@yesmeck yesmeck reopened this Dec 23, 2016
@yesmeck yesmeck self-assigned this Dec 23, 2016
@yesmeck
Copy link
Member

yesmeck commented Dec 23, 2016

@piotrwitek Do you know why generic type doesn't work on static prop, but works on exported class? Any links to the typescript doc?

@piotrwitek
Copy link
Author

piotrwitek commented Dec 23, 2016

Thanks for reopening the issue 👍
@yesmeck The error from compiler state that:

Static members cannot reference class type parameters

I found this explanation:
A class has two sides to its type: the static side and the instance side. Generic classes are only generic over their instance side rather than their static side, so when working with classes, static members can not use the class’s type parameter.
https://www.typescriptlang.org/docs/handbook/generics.html

@BANG88 I saw the link, but I believe this use scenario is not optimal, because you should provide a consistent way to use your components with a generic interface, and if you can extend Table with generic type, and cannot extend generic Column in the same way, you are actually breaking the consistency of your interface

@yesmeck
Copy link
Member

yesmeck commented Dec 23, 2016

@piotrwitek What typescript version are you using?

@piotrwitek
Copy link
Author

@yesmeck typescript@2.1.4

tsconfig.json

{
  "compilerOptions": {
    "strictNullChecks": true,
    "noImplicitAny": true,
    "noImplicitThis": true,
    "noImplicitReturns": true,
    "noFallthroughCasesInSwitch": true,
    "noUnusedLocals": true,
    "noUnusedParameters": false,
    "preserveConstEnums": true,
    "allowUnusedLabels": false,
    "allowUnreachableCode": false,
    "allowJs": false,
    "allowSyntheticDefaultImports": true,
    "noEmit": false,
    "noEmitOnError": true,
    "noEmitHelpers": false,
    "importHelpers": false,
    "experimentalDecorators": true,
    "emitDecoratorMetadata": true,
    "pretty": true,
    "declaration": false,
    "isolatedModules": false,
    "module": "commonjs",
    "moduleResolution": "node",
    "noResolve": false,
    "outDir": "../out",
    "newLine": "LF",
    "removeComments": true,
    "sourceMap": true,
    "target": "es5",
    "jsx": "react",
    "lib": [
      "dom",
      "es2016",
      "es2017.object"
    ]
  },
  "include": [
    "**/*"
  ],
  "exclude": [
    "node_modules/**/",
    "jspm_packages/**/"
  ],
}

@piotrwitek
Copy link
Author

piotrwitek commented Dec 23, 2016

@yesmeck
I believe the possible solution is to update the Column Class to be possible to extend it as generic class in the same way as with Table, that would be consistent and elegant enough.

I agree with @BANG88 provided example above, that the following should work and would fix the issue without need to extract Column or other ugly workarounds:

// Define a  generic Table 
class MyTable extends Table<IPhoto>{}
// Define the generic Column.
class MyColumn extends Table.Column<IPhoto>{}

EDIT: But the optimal solution would be for the Column to get generic type out of the extended generic MyTable class so following would work as expected:

<MyTable ... >
  <MyTable.Column ... /> // should have generic type inferred from MyTable automatically
</MyTable>

@piotrwitek piotrwitek changed the title Export Column as component to leverage generic types similarly as in Tables Allow Table.Column class type to be possible to extend as a generic type in TypeScript Dec 23, 2016
@piotrwitek
Copy link
Author

@yesmeck I have updated my proposal, please check it for details of possible solutions

@yesmeck
Copy link
Member

yesmeck commented Dec 26, 2016

@piotrwitek Could you send a PR, if we can solve this issue by not exporting Column, I'd like to +1.

@piotrwitek
Copy link
Author

@yesmeck sure I'll take a look at this in coming days

@benjycui
Copy link
Contributor

benjycui commented Jan 3, 2017

@yesmeck how about this, use getColumnConfig instead of read props from it directly?

class TableColumn extends React.Component {
  getColumnConfig() {
    ...
  }
}

@afc163
Copy link
Member

afc163 commented Jan 10, 2017

Add some doc about Using Table in TypeScript

@yesmeck
Copy link
Member

yesmeck commented Jan 10, 2017

@benjycui 没看懂。

@afc163
Copy link
Member

afc163 commented Jan 15, 2017

@yesmeck Could it to be closed?

@yesmeck yesmeck closed this as completed Jan 15, 2017
@yesmeck
Copy link
Member

yesmeck commented Jan 15, 2017

@piotrwitek Try the latest version, Table.Column can be extended now.

@lock
Copy link

lock bot commented May 2, 2018

This thread has been automatically locked because it has not had recent activity. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock bot locked as resolved and limited conversation to collaborators May 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants