-
-
Notifications
You must be signed in to change notification settings - Fork 262
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
feat(data-table): support generics #1954
Conversation
src/DataTable/DataTable.svelte
Outdated
* @typedef {string} DataTableKey | ||
* @generics {Row extends DataTableRow = DataTableRow} Row | ||
* @template {DataTableRow} Row | ||
* @typedef {Exclude<keyof Row, "id">} DataTableKey<Row=DataTableRow> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The key type must disallow "id"
being used as value, as that is a reserved prop for rows
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a reason why the id
would not also be displayable as a column?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point
I have listed a few issues here that come up when typing keys like this. Looks like you also ran into one with having to add a property for an empty column. There also can be columns that aggregate data from multiple properties or columns accessing nested properties (more detail on that in the linked comment). |
@brunnerh Thanks for the fast reply and the feedback. Looking at this more closely (especially nested keys and empty columns), this seems less feasible. |
I think supporting nesting up to a low level (maybe just one level deeper) would be OK. Also for empty columns, the key type could potentially be changed based on the (I can maybe look at this more closely and play around with it at another point.) |
Some observations: Strict typing of the Upon further thought, changing any types for the Property path resolution could be integrated but would likely require a separate // item type:
{
id: number;
name: {
first: string;
last: string;
};
deep: {
deeper: {
deepest: string; // <= not in suggestions
};
};
age: number;
} Experimental changes: 2b6c7b0 The path resolution could probably be extended to also resolve the type of I would say the most important thing when adding generics to this component is the typing of the Another thing that could be considered would be to use a generic just for the row keys which is based on the item type but also gets loosened to all strings. This type then could be used in the ( |
@brunnerh That's awesome – thank you for your work on this. Agreed that the Would you mind opening a PR against this one with your change? |
It's a bit wonky, but for multiple generics, the values cannot be space-separated: /**
* @typedef {{ key: T1; value: T2; }} Typedef<T1,T2>
* @generics {T1, T2} T1,T2
*/ |
src/DataTable/DataTable.svelte
Outdated
@@ -5,7 +5,7 @@ | |||
* @typedef {import('./DataTableTypes.d.ts').PropertyPath<Row>} DataTableKey<Row=DataTableRow> | |||
* @typedef {any} DataTableValue | |||
* @typedef {{ | |||
* key: DataTableKey<Row>; | |||
* key: DataTableKey<Row> | string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reasoning for this?
Though I also question the necessity for the split into two header types, it feels like there could just be one with an optional value
property.
(To retain auto completions on properties, this should be | (string & {})
as on the DataTableCell
key.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amended per your suggestion in 3407684
It was to allow the keys of empty table headers to not be included in rows. See cd90958
const headers = [
{ key: "name", value: "Name" },
{ key: "port", value: "Port" },
{ key: "rule", value: "Rule" },
{ key: "overflow", empty: true }, // Keys in empty table headers should not be required as properties in `row`
] as const;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it is a bit more common that columns without headers have some special content and are less likely to be bound to a specific property.
@brunnerh Thank you for the help. Let me know if my additional commits look good. I think this is a potential "breaking change" for TS users (although arguably it's a fix since it's now enforcing types). Regarding adoption, my preferred recommendation for TS users is to use a const assertion for 1. Use a const assertion
const headers = [
{ key: "name", value: "Name" },
{ key: "port", value: "Port" },
{ key: "rule", value: "Rule" },
] as const; 2. or pass
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I'd say.
3407684
to
2a66bf8
Compare
2a66bf8
to
9b52ee8
Compare
9b52ee8
to
ab4fe57
Compare
ab4fe57
to
dee7f38
Compare
dee7f38
to
92f14fa
Compare
Adjust DataTable types. - Make key in `cell` slot prop less strict to prevent type errors in markup. - Resolve property path names up to one level deep for header keys.
92f14fa
to
8578c6d
Compare
Apologies for the long delay on this one. This is released in v0.86.0. Thank you @KoichiKiyokawa and @brunnerh for the tremendous work here to greatly improve the DX of using |
This PR is based on #816. Closes #816
sveld
to v0.20 to use genericsDataTable
so that types forrows
andheaders
can be inferred@brunnerh Would love your thoughts on this. Seems like a straightforward win. I love the inferred types based on keys in
rows
. FWIW, this doesn't break existing usage (see theDataTable.test.svelte
file) of exported props, since the generic parameter is optional (<Row = DataTableRow>
).