carbon-design-system / carbon-components-svelte

Svelte implementation of the Carbon Design System
https://svelte.carbondesignsystem.com
Apache License 2.0
2.64k stars 256 forks source link

DataTable runtime row type is lost #667

Open brunnerh opened 3 years ago

brunnerh commented 3 years ago

The DataTable component uses a spread operation internally which loses all non-enumerated properties and the original type:

$: rows = rows.map((row) => ({
  ...row,
  cells: headerKeys.map((key, index) => ({
    key,
    value: resolvePath(row, key, ""),
    display: headers[index].display,
  })),
}));

E.g. functions of the prototype will be gone:

var x = { log() { console.log('!') } }
var y = { ...x }
y.log();
// logs !

class X { log() { console.log('!') } }
var x = new X();
var y = { ...x }
y.log()
// TypeError: y.log is not a function

I would recommend assigning the original row to a property instead of or additionally to spreading it to prevent this.

In my particular use case the row is a Proxy, which becomes mostly useless after the spread. Having the original type might also be useful for type-based operations using instanceof.

brunnerh commented 3 years ago

Also, this merge operation overwrites any cells property on the row, which could potentially lead to unexpected behavior when trying to access this via the injected row:

<script>
  import { DataTable } from "carbon-components-svelte";

  const headers = [
    { key: 'organism', value: "Organism" },
    { key: 'cells', value: "Cells" }
  ];

  const rows = [
    { id: 0, organism: "Elegans - Adult male", cells: 1031 },
    { id: 1, organism: "Elegans - Adult hermaphrodite", cells: 959 },
    { id: 2, organism: "Elegans - Hatched larvae", cells: 558 },
  ];
</script>

<DataTable {headers} {rows} title="row.cells unexpected">
  <span slot="cell" let:row let:cell>
    {#if cell.key == 'cells'}
      {row.cells}
    {:else}
      {cell.value}
    {/if}
  </span>
</DataTable>

image

metonym commented 2 years ago

In https://github.com/carbon-design-system/carbon-components-svelte/pull/1179, the implementation changed so that rows is not modified. cells is no longer written to the rows prop.

However, this is still an issue if using slotted cells, as row.cells will still override the cells property. This is addressed by 5d61929.

metonym commented 2 years ago

@brunnerh Can you provide a simple repo of your use case to illustrate the first problem?

brunnerh commented 2 years ago

@metonym Do you mean the type being lost?

This illustrates two use cases when using classes: REPL

I currently cannot access the original project using proxies, but this should be pretty similar: REPL The cell value is read correctly from the underlying proxy but the row cannot be used to read or update the value. To fixe the marked errors, the underlying data or the original rows have to be accessed instead. E.g. row[cell.key][row.property] => data[cell.key][row.property]