Few questions around contributing and question around transactions

[amaidah]

Hi Nicolas, first of all I'd like to thank you on building a really neat grid library with a simple data structure and easy to read documentation. The concept of making ID core to the library is really useful and solves a lot of use cases.

How open are you to PRs and contributions on this project? I'd be happy to contribute if it makes sense. There isn't much here but I see that the project is hosted inside the Equify repo so I'm wondering if the roadmap is more or less set by the company's needs?

For instance, I'd like to build "views" similar to how it's done in Airtable/Notion and I've put a rough example here: https://codesandbox.io/s/musing-meadow-ueorc?file=/src/App.js. I'd like to store the data on the BE so that the FE has no idea that it's rendering a "view". With the BE responsible for view logic, FE just takes the provided list and renders into the grid. However, from what I can tell, onChange exposes the entire new dataset instead of the transactional diff. This can make it tricky for instance when you delete a row from a view and need to track that back to the main list.

To make this simpler, I am imagining something like having transaction records maybe something like:

{action: 'DEL', ...{id: 1234, // row data }
{action: 'SET', ...{id: 1235, // row data }

Otherwise the logic to diff on update/create/deletion can get complex especially so if multiple clients start to query the BE.

I took a peak through the source code and I noticed some spots where a potential new callback or exposing a parameter could work:

const setRowData = useCallback(
  (rowIndex: number, item: T) => {
    console.log("SET", item); // expose via new callback

    onChange([
      ...dataRef.current?.slice(0, rowIndex),
      item,
      ...dataRef.current?.slice(rowIndex + 1),
    ]);
  },
  [onChange]
);

or

const setRowData = useCallback(
  (rowIndex: number, item: T) => {
    onChange(
      [
        ...dataRef.current?.slice(0, rowIndex),
        item,
        ...dataRef.current?.slice(rowIndex + 1),
      ],
      {action: 'SET', ...item} // potentially add as second parameter not to break current API
    ); 
  },
  [onChange]
);

const deleteRows = useCallback(
  (rowMin: number, rowMax: number = rowMin) => {
    if (lockRows) {
      return;
    }

    setEditing(false);
    setActiveCell((a) => {
      const row = Math.min(
        dataRef.current.length - 2 - rowMax + rowMin,
        rowMin
      );

      if (row < 0) {
        return null;
      }

      return a && { ...a, row };
    });
    setSelectionCell(null);

    for (let i = rowMin; i <= rowMax; i++) {
      console.log("DEL", dataRef.current[i]); // each transaction could be exposed via new callback
    }

    onChange([
      ...dataRef.current.slice(0, rowMin),
      ...dataRef.current.slice(rowMax + 1),
    ]);
  },
  [lockRows, onChange, setActiveCell, setSelectionCell]
);

or

const deleteRows = useCallback(
  (rowMin: number, rowMax: number = rowMin) => {
    if (lockRows) {
      return;
    }

    setEditing(false);
    setActiveCell((a) => {
      const row = Math.min(
        dataRef.current.length - 2 - rowMax + rowMin,
        rowMin
      );

      if (row < 0) {
        return null;
      }

      return a && { ...a, row };
    });
    setSelectionCell(null);

   const deletedRows = [];
    for (let i = rowMin; i <= rowMax; i++) {
      deletedRows.push({ action: 'DEL', ...dataRef.current[i] }); // create list of deletedRows
    }

    onChange([
      ...dataRef.current.slice(0, rowMin),
      ...dataRef.current.slice(rowMax + 1),
    ], 
    [...deletedItems] // potentially exposed here
   );
  },
  [lockRows, onChange, setActiveCell, setSelectionCell]
);

Have you thought about this idea at all? What do you think? Or maybe there's somewhere better to expose this if you're into the idea?

[nick-keller]

Hello @amaidah and thanks for the interest!

Contributing

I am more than open to contributions, especially where the library is lacking:

• Unit tests
• CI
• Making it more contribution-friendly
• Documentation
• Marketing

The repo is hosted under the company's name because it started off as an internal project but could be moved if that makes sense. I try to be careful with the choices that are made so discussing it before coding is recommended, especially with things that can already be done:

• should we include it and impose in the core library,
• should it be opt-in,
• should it be a separate repo?

This being the core lib I'd like to keep it as agnostic as possible and let people build opinionated wrappers around it.

Row ids

The concept of making ID core to the library is really useful and solves a lot of use cases.

If you are talking about ids on rows, this is already feasable. I did not want to include it in the core and impose it for a few reasons that can be challenged:

• Not needed for the core lib
• Not useful for everyone
• In some cases, primary keys are tuples of multiple columns
• Dead simple to implement on top of the core lib (cf example in the doc)

Actions / transactions

Now regarding your other question, I really like the idea of being able to track "actions". But I feel like building it right in the core of the lib (as in your examples) is a bit risky: everywhere that we call onChange we need to do the same thing a bit differently twice (compute the new data and the "actions"). This means:

• Harder to maintain
• More opportunities for bugs

Another downside of doing it here is that the actions represent the user's actions, not the actual diff. So updating a cell with the same value would trigger an action, even when the diff is empty. (You could argue that this is the desired result ^^)

An alternative implementation would be to wrap the onChange callback and compare the received value to the actual value: do a diff based on the row's primary key to find added, updated, and deleted rows, then compare the rows to find which fields were updated precisely (only if needed, for updated rows).

In short, I am more than open to discussions, especially with people like you that have concrete use cases. I am not shutting down the idea, I just want to make sure it is executed properly ;)

So feel free to challenge what I just said and we can come up with the best solution to that problem (you are not the first one to request such a behavior)

[amaidah]

Hi @nick-keller, thanks for the quick reply and I'm really glad to hear you're open to contributions.

About row id's, I saw that example in the documentation and I meant it as a thank you that the concept is available in the library. I wasn't suggesting to move it to the core lib-- in fact I didn't realize it wasn't in the core lib, I thought it was because it was in the docs!

An alternative implementation would be to wrap the onChange callback and compare the received value to the actual value: do a diff based on the row's primary key to find added, updated, and deleted rows, then compare the rows to find which fields were updated precisely (only if needed, for updated rows).

Could you expand more on this? Are you suggesting that tracking the actions should happen outside the library? Or are you suggesting to implement a wrapper ontop of onChange inside the library? I was thinking tracking actions should be dealt with by the library itself.

About exposing the actions where I had suggested it in those methods, that was the first idea that came to my mind, because we already know what kind of action it is and we are already doing some logic to determine the diff in those methods. However, it also makes sense to centralize the logic as you said.

[nick-keller]

Regarding wrapping the onChange prop:

return (
  <DataSheetGrid
    value={data}
    onChange={(newData) => {
      const deletedRows = new Set(data.map((row) => row.id))
  
      for (const row of newData) {
        deletedRows.delete(row.id)
      }
  
      const createdRows = new Set(newData.map((row) => row.id))
  
      for (const row of data) {
        createdRows.delete(row.id)
      }
  
      console.log('deletedRows', deletedRows)
      console.log('createdRows', createdRows)
  
      setData(newData)
    }}
  />
)

By comparing newData with current data you can easily extract added / removed rows. You could go further and use a deepEqual to compare rows between newData with current data to find which rows were updated. And by comparing each prop individually you could even know which keys where updated.

[amaidah]

Thank you for the suggestion, I implemented something similar to this and it works. One concern I can think of with this approach is that you'll need to run all 3 transactional diffs on every change, which can have a performance impact at some point. Versus what I had initially suggested, we'd know what's been done and we just expose at the same time the calculations are being made and we'd only run the one method that's needed. That said for my use case, the data is not huge so this will work.

  const handleSetData = (nextData) => {
    const deletedRows = transactions.getDeletedRows(data, nextData);
    const createdRows = transactions.getCreatedRows(data, nextData);
    const updatedRows = transactions.getUpdatedRows(data, nextData);

    setData(nextData);
  };

[nick-keller]

This is now available in 3.4.0, please read the doc for onChange