feat(data-table): type using generics

[metonym]

This PR is based on #816.

• Ugprades sveld to v0.20 to use generics
• Uses generics to type DataTable so that types for rows and headers 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 the DataTable.test.svelte file) of exported props, since the generic parameter is optional (<Row = DataTableRow>).

[metonym]

The key type must disallow "id" being used as value, as that is a reserved prop for rows.

[brunnerh]

I don't see a reason why the id would not also be displayable as a column?

[metonym]

Good point

[metonym]

If using empty table headers, an explicit key must be set on rows:

[brunnerh]

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).

[metonym]

@brunnerh Thanks for the fast reply and the feedback. Looking at this more closely (especially nested keys and empty columns), this seems less feasible.

[brunnerh]

I think supporting nesting up to a low level (maybe just one level deeper) would be OK.
Maybe adding another generic to the header to bail out more easily would be good (so it defaults to determining valid keys but can be set to string).

Also for empty columns, the key type could potentially be changed based on the empty property 🤔.

(I can maybe look at this more closely and play around with it at another point.)

[brunnerh]

Some observations:
I don't think that synthetic keys are an issue for the headers definitions in the <script>; usually either the headers will be more loosely typed in JS or one is using TS directly in which case 'key' as any can be used in those cases.

Strict typing of the cell slot prop key is not so great, though.
Comparisons with strings that are not among the known values will cause an error ("This comparison appears to be unintentional because the types '...' and '...' have no overlap."). In Svelte 5, TypeScript syntax will be possible in the template but right now this could be annoying to deal with.
The property could be typed in way that you still get valid property suggestions.

Upon further thought, changing any types for the empty columns does not make much sense to me, the cells behave essentially the same, just the header is different.

Property path resolution could be integrated but would likely require a separate .d.ts.
With one level deep:

// 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 cell.value and the like.

I would say the most important thing when adding generics to this component is the typing of the row slot property which allows safe access of all the data in the items. (This is already working nicely.)

---

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 cell.key to get all keys that were actually defined as part of the headers; the key access in the template then would be as strict and correct as it possibly can be.

(sveld does not appear to support multiple generics at the moment, though.)

[metonym]

@brunnerh That's awesome – thank you for your work on this. Agreed that the rows being typed is most important.

Would you mind opening a PR against this one with your change?

[metonym]

sveld does not appear to support multiple generics at the moment, though.

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
 */

[brunnerh]

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.)

[metonym]

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;

[brunnerh]

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.

[metonym]

@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 headers. An alternative would be to pass headers inline. I hesitate to recommend directly using the DataTableHeader<typeof row[0]> approach as I feel it is less elegant and more verbose.

1. Use a const assertion

headers expects a read-only array.

const headers = [
  { key: "name", value: "Name" },
  { key: "port", value: "Port" },
  { key: "rule", value: "Rule" },
] as const;

2. or pass headers inline

<DataTable
  headers={[
    { key: "name", value: "Name" },
    { key: "port", value: "Port" },
    { key: "rule", value: "Rule" },
  ]}
/>

[brunnerh]

Looks good, I'd say.