Make TS TableRecord interface contain the record response instead of extending it

[seancolsen]

Problem

Here we have the following code:

interface TableRecordInResponse {
  [key: string]: unknown,
}

export interface TableRecord extends TableRecordInResponse {
  __identifier: string,
  __isAddPlaceholder?: boolean,
  __isNew?: boolean,
  __isGroupHeader?: boolean,
  __rowIndex?: number,
  __state?: string,
  __groupValues?: Record<string, unknown>,
}

That means that for one table cell, I might have an object like this:

{
  id: 1,
  Common Name: "Coast redwood",
  latin_name: "Sequoia sempervirens",
  height_in_meters: 115.92,
  name: "Hyperion",
  class: "Conifer",
  country: "United States",
  __identifier: "__0_normal_0",
  __rowIndex: 0,
  __state: "done",
}

This design leaves our internal front-end state data intermingled with the user data from postgres. What if I have a column named __state?

Example bug

1. Go to "New Table" > "Import Data".

2. Set "Import From" to "Clipboard".

3. Paste the following:

   task,__state
   Foo,done
   Bar,error
   Baz,loading
   Bat,idle

4. Click "Continue".

5. Click "Finish Import".

6. When the table loads, observe "done" printed for every row in the table.

7. Expect to see "done", "error", "loading", and "idle" values in the rows respectively.

There's all sorts of other strange behavior when a column shares a name with one of our internal fields.

Solution

Instead of TableRecord extending TableRecordInResponse, I think TableRecord should contain TableRecordInResponse as follows:

interface TableRecordInResponse {
  [key: string]: unknown,
}

export interface TableRecord {
  record: TableRecordInResponse, // <-- new property
  identifier: string,
  isAddPlaceholder?: boolean,
  isNew?: boolean,
  isGroupHeader?: boolean,
  rowIndex?: number,
  state?: string,
  groupValues?: Record<string, unknown>,
}

This design also eliminates the need for the double underscores.

Notes

I'm marking this "draft" until we get an okay from @pavish

[pavish]

@seancolsen The solution mentioned sounds good to me. We may also want to restructure the TableRecord interface to better suit our new grouping model.

[seancolsen]

Thanks @pavish I'll try to work on this as part of the front end changes for #862

[seancolsen]

@pavish I decided not to get into this today because I didn't want to add more complexity than necessary to the range grouping PR. We can take up this refactoring later.

[pavish]

@seancolsen That's fine. I was specifying the structure changes based on the new grouping model as a potential refactor we can do when this issue is picked up, we don't have to prioritize this right away.