Add rowKey option and documentation

[robgaston1]

This solves #715 (comment)

If you add data rows only to the end of your table, the latest release of vue-good-table will maintain the expanded state of all rows, and not collapse them automatically any time the data changes. But if you add a row in the middle of the table, the one that was added is automatically expanded and the one that is bumped down gets collapsed.

Proposed Solution
To the groupOptions, add an optional rowKey attribute, which is used to provide a unique identifier for the row data. If provided, that key will be used to track expanded state instead of the vgt_header_id.

[TheJaredWilcurt]

@xaksis ran this locally. fixes the issue. This can be merged/build/release. 👍

[xaksis]

hey @TheJaredWilcurt I don't think this should be an external input. This is a legit bug in the table and the table should use unique ids for each row by default.

[TheJaredWilcurt]

@xaksis

Currently it looks like the vgt_header_id is just assigned to an index:

• vue-good-table/src/components/Table.vue

  Line 1372 in a7154e7

  headerRow.vgt_header_id = i;

This PR solves if the data passed in changes, by adding or removing a value at the top or in the middle. Due to Vue's reactivity, the only way for it to be able to handle this is if the existing data has unique keys for Vue to match against. But since VGT is not providing the data, it can't accurately know if the data is the same or has changed. There are only 3 ways to handle this:

• Terrible idea: Use a watch to see when the props change, and then loop over both the new and old values, doing a deep comparison with something like lodash's _isEqual, to then apply the same key to the new values as the old if they are an exact match, and create new keys for the new data.
• Terrible idea: JSON.stringify the entire row of data and either use that gigantic string directly as the key, or pass it into a hashing function.
• Simplest solution: What was merged in here seems to me to be the simplest option. It still works fine by default for those adding/removing from the bottom of a list, and for those inserting/removing from above that, they have an extra requirement of supplying a key, which is how Vue works for lists, so it's not an unexpected or difficult demand to meet.

If there is another solution that I'm not seeing, let me know.

[xaksis]

A solution would be to use a unique id for vgt_header_id (for example, based on timestamp) instead of using index (i).
This PR puts the burden on users to specify a unique id, which I don't think is correct. It is a simple fix on our end. I'll add that in the next release.

[TheJaredWilcurt]

If I have this array:

[
  { kitten: 'r' },
  { kitten: 'd' }
]

and I pass it in, then VGT will do a deep clone of it and produce this:

[
  { kitten: 'r', vgt_header_id: 1591814658713 },
  { kitten: 'd', vgt_header_id: 1591814658714 }
]

Then later I change the data I am passing in

[
  { kitten: 'r' },
  { kitten: 'y' },
  { kitten: 'd' }
]

Then VGT will deep clone it and produce new keys.

[
  { kitten: 'r', vgt_header_id: 1591814802077 },
  { kitten: 'y', vgt_header_id: 1591814802078 },
  { kitten: 'd', vgt_header_id: 1591814802079 }
]

The keys for ALL of these values have changed, defeating the purpose of using keys. Vue needs to be able to know that the DOM element it has in the virtual DOM maps to the correct piece of data, by ensuring the keys are the same. In this case, it assumes that all rows must be replaced because there are no matching keys, so the data is completely fresh. When in reality, all that happened was the insertion of one new item.

Also when looping over and assigning timestamps, depending on the speed and workload on the CPU you could get dozens of matching timestamps. Instead you would want a helper like this to create a temp ID:

// Produces a temp ID like: 'temp_b0srg5u6k_1590157296289'
function generateTempId () {
  let prefix = 'temp';
  let random = Math.random().toString(36).substr(2, 9);
  let date = Date.now();

  return [
    prefix,
    random,
    date
  ].join('_');
}

But even that won't work, because of the reasons mentioned above. Everytime the data being passed in changes at all, the ID's would all be re-created. So Vue won't be able to match against the existing virtual DOM.

You need to either:

1. Manually compare what is being changed and copy the existing ID's to matching rows (slow, bad, do not do this).
2. Produce a key based on the content by stringifying the row (bad, slow, heavily error proned as many data-types are not JSON stringify friendly)
3. Let the user pass in a unique key

[TheJaredWilcurt]

I forgot to mention, the timestamp approach is worse than the index approach. In the index approach, there are at least some cases (adding a new item to the end of a list, or removing the last item from a list) where it works as expected. Where as with the timestamp or randomly generated ID it would break in all scenarios.

The randomly generated ID would only work if you applied it to the data being passed in before it got to Vue-Good-Table, since the external parent component would have the context of what changes are occurring to the list and when it needs to generate a new ID for a new item being added to the list.

[xaksis]

Are you concerned about the render performance? because, as long as at any given time, all rows have a unique id, it would solve the current issue that this PR solves? It doesn't matter what the ids were before. But you're right that it would have to re-render the table but I don't see how this would break anything?

When a row is inserted or deleted, we currently do a deep copy of the rows. the only difference would be that the currently visible header rows will be re-rendered. Is this what your concern is?

Where as with the timestamp or randomly generated ID it would break in all scenarios.
I'm not sure how this would break anything?

The timestamp was just a hypothetical example, of course, there are other ways of getting unique ids: https://gist.github.com/gordonbrander/2230317

[xaksis]

Just did a quick test with my usual:

getId() {
      return Math.random().toString(36).substring(2)
        + (new Date()).getTime().toString(36);
    },

Works with:

• Collapsing/Expanding
• filtering
• sorting
• inserting a row
• deleting a row
• selecting rows

[xaksis]

Ah crap! I am so sorry, I actually totally misread the posted issue!

But if you add a row in the middle of the table, the one that was added is automatically expanded and the one that is bumped down gets collapsed.

It is about maintaining collapse/expand state!! Whoops. I thought it broke collapsing/expanding!

[xaksis]

@TheJaredWilcurt now I see what you were talking about. Sorry about being obtuse. You're saying the collapse/expand state will not be maintained if vue doesn't know what the ids were before we refreshed the id completely! Got it.

Apologies again. I'll look over this once more, coming weekend and release if everything looks good.

[TheJaredWilcurt]

@xaksis If this is good, can we get a release? Otherwise, lemme know if you wanted other changes

[xaksis]

I'm waiting for another PR, if it doesn't make it in by today. I'll release what we have.

[robgaston1]

@xaksis thank you for merging this and putting it in the latest release. However, after upgrading to this last release, it looks like the new code is not being applied.

One way I noticed this is that there is a computed property in the new code called rowKeyField. It doesn't show up in the Vue dev tools when running the latest release.

I pulled down the latest version of the code to my locally forked repo, and when I ran npm run bundle, that computed property now shows up and the new code from this pull request does indeed seem to be working. Is it possible the dist didn't get updated before this release? Thanks!

[xaksis]

@robgaston1 I have made every bungle possible with this PR :( - v2.19.5 should have the updated bundle.

[robgaston1]

@xaksis no problem, these things happen! Thanks for being responsive to our requests for releases. 👍