[Svelte] Table does not consider key property

[zonradkuse]

Describe the bug
It seems like the order of the headers matters and that the respective key property is not used to select the cells. See the following snippet to illustrate the problem.

To Reproduce
Here an example snippet

<main>
<Table {...data} />
</main>
<script>
    import 'agnostic-svelte/css/common.min.css';
    import { Button, Table, Loader, Alert } from 'agnostic-svelte';
    let data = {
        rows: [
            {"score": 10, "team_name": "my team"},
            {"score": 20, "team_name": "my team 2"},
        ],
        headers: [
            {
                label: 'Team',
                key: 'team_name',
                width: 'auto',
                sortable: true,
            },
            {
                label: 'Score',
                key: 'score',
                width: 'auto',
                sortable: true,
            }
        ],
    }
</script>

This results in the following table:

Team	Score
10	my team
20	my team 2

Expected behavior
The snippet should result in

Team	Score
my team	10
my team 2	20

Desktop (please complete the following information):

• OS: Fedora 34
• Browser: Firefox
• Version 98

[roblevintennis]

This is an interesting point. Honestly, the .key was introduced just to support sorting e.g. we have:

const headerWithCustomSortFunction = headers.find(
   (h) => h.key === sortingKey && !!h.sortFn
);

But I think you make a good point that if you've bothered to add header[index].key the rendering should respect that.

I really wish we were already ported to Typescript for Svelte (on the horizon); because I'm trying to think about the case where the data is inconsistent with itself e.g. I see some possible cases:

1. headers with objects that contain no .key -- easy, we use the corresponding ordering between headers/rows (as it appears we're already doing; so maybe this is the default fallback behavior)
2. headers with objects that DO contain a .key. In that case we could/should map the render ordering accordingly (please correct me if I'm wrong but I think this is the issue you're reporting yah?)
3. But what if they inconsistently supply .key for only certain headers objects. Or, if they mismatch the naming such that the key doesn't exist in the rows? Or something else that's a user error. Do we just understandably have undefined results and be sure to say so much in the docs? Or, do we throw errors to help the application developer find bugs in their code? I think the later is considered best practice when possible. Or something else ❓

Perhaps we don't need to solve for 3. at all and can just do case 1. and case 2. which would certainly be an enhancement over what's there today; and log a separate ticket or do a separate iteration for the proper error handling. Thoughts?

@zonradkuse Is this something you'd be interested in tackling yourself were you just reporting? We'd be gracious if it's the former 😉 but also graciously accept a valid issue as too 😄 lmk

Marking as both a bug and enhancement as this is sort of a mix of both :)

[roblevintennis]

Also @zonradkuse if you're interested I've just started https://gitter.im/AgnosticUI/contribute a few days back and I'm down to chat over some of these things. I'm hit and miss whether in there but I get email notifications and pop in a few times daily.

[zonradkuse]

I see! I believe that this is a naming issue as key is ambiguous in this context. I thought key was meant exactly for that and I wasn't aware it is for sorting (I did not see this in the docs, this doesn't mean it is not there, though).

I retrieve data from an API endpoint where I can not guarantee the order of the elements. Depending on the backend, the order depends on the dictionary implementation or their serialization. So, the feature would be awesome to have.

You raise a valid point in case 3. I believe, a cleaner API design could use another property selectRowValueByKey (better name?) with default false (to not break any existing code). Only then the code should enforce the key property. WDYT?
Another solution could be to rename the key property to something like sorting_key to get rid of the ambiguity.

I have to think about this for a bit. It's pretty late here already.

For now I have a rather dirty 3-minute 6 LOC solution:

        let rows = [];
        for (const row_index in results) {
            const r = results[row_index];
            let row = {};
            for (const h_index in data.headers) {
                const head = data.headers[h_index];
                row[head.key] = r[head.key];                
            }

            rows.push(row);
        }

Right now, I have more work on my desk than I am able to tackle and my js is a bit rusty. I can give it a go but can't promise any results in the next few weeks. It does not seem to be that complicated, though.

[Croug]

I can go ahead tackle this as I think it'd be pretty simple, this would also open the door for more selective data visibility, so if the table is supplied data with a key not present in the headers, it can simple be ignored, this is actually an issue I ran into initially when using the table

[roblevintennis]

Thanks y'all @Croug 🙏

[zonradkuse]

Thank you! :-)

[roblevintennis]

@Croug Is this something you think you'll be able to tackle still? I know you're busy w/the exam prep.

[Croug]

Yup I can still tackle this, I'll be finishing up my cert here in the next few days and I'll have plenty of time then if I don't get to it before then

[roblevintennis]

This blocks #213 which should probably come right after (we can port to other frameworks after the Astro hackathon project is submitted)

[roblevintennis]

@zonradkuse my understanding is that #197 already addresses this but we just want to add a storybook story to confirm. But, if this is a blocker for you feel free to cherry-pick that in and confirm it works on your end. I think that PR is slated to land soon though — thanks for bearing with us!