Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EuiSwitch] Not re-rendering on toggle in EuiDataGrid #4175

Closed
j-m opened this issue Oct 23, 2020 · 7 comments
Closed

[EuiSwitch] Not re-rendering on toggle in EuiDataGrid #4175

j-m opened this issue Oct 23, 2020 · 7 comments

Comments

@j-m
Copy link
Contributor

j-m commented Oct 23, 2020

EuiSwitches in a column (not leading/trailing) of an EuiDataGrid do not re-render on toggle.
The EuiSwitch I click on is not re-rendered until I go to another page and back.

I added console.logs and clicking definitely triggers the render function of the EuiDataGrid and the callback of the EuiSwitch.
The state of the EuiSwitch is stored in the EuiDataGrid and is rendered in the renderCellValue function like so:

...
<EuiSwitch
  compressed
  style={{ padding: 6 }}
  label={<></>}
  checked={this.state.rows[rowIndex].Ready}
  onChange={(event: EuiSwitchEvent) => {
    const { checked } = event.target
    this.setState((state: SequencesTableState) => {
      const rows = [...state.rows]
      rows[rowIndex].Ready = checked
      return { rows }
    })
  }}
/>
...

I was looking at the implementation of EuiSwitch and I don't understand how it updates its own checked state, I expected something closer to

checked = !checked
event.target.checked = checked;

The project I'm working on is my first experience with React, so I welcome any suggestions/explanations :)

@chandlerprall
Copy link
Contributor

I put together a quick demo showing the state updates triggering a re-render https://codesandbox.io/s/hopeful-cherry-ot1im?file=/index.js

However, I don't recommend setting up the state/data quite that way, as tying renderCellValue to the state like that demo does will cause the entire grid to re-render when any row's selection changes. If you can reproduce your selection issue in a code sandbox I'd be happy to look into & explain why the switch doesn't toggle until you reload the page, and provide suggestion(s) for connecting your data to the grid. (This is a topic that our datagrid docs need a whole section added around.)

@j-m
Copy link
Contributor Author

j-m commented Oct 26, 2020

Okay, thank you!
Just to add a few more details, I'm using TypeScript and a PureComponent and don't use anything fancy like useMemo.
I'll try and create a MCVE - not used CodeSandbox before so this could be interesting...

@j-m
Copy link
Contributor Author

j-m commented Oct 26, 2020

Okay I've managed to reproduce it:
https://codesandbox.io/s/adoring-fast-wi812?file=/src/App.tsx
Just moved things about and culled a lot of code, refactor however you see fit

@chandlerprall
Copy link
Contributor

Thanks! Here's a quick walk-through of what's going on:

Your setup stores whether the row is selected on the individual items' Ready value. When you click a switch, the component correctly modifies that row's Ready state, which causes a re-render of the whole component. However, when that triggers the datagrid's render, it checks to see if relevant props it receives have changed and only permits the re-render if that is true. In your case, none of the props passed in changed in response to the state change, and the re-render is ignored. Switching to another page of the data and back forces a re-render, so the updated switch status reflects the current state when moving between pages.

Conceptually, it's important to remember your renderCellValue is another component that happens to exist on the Table class, which gives it access to Table's state. But React doesn't know about that relationship, it doesn't know the renderCellValue component "shares" the Table's state. To React the tree looks like:

Table
  EuiDataGrid
    renderCellValue

and the only way to get update a cell is let React know something in renderCellValue has changed (again, the this usage in your renderCellValue is the Table, so this.setState only directly affects the Table.

The best way to work with dynamic data in the datagrid is to provide it via Context. This can be done directly using React's createContext or by using a library such as recoil or MobX. Then the renderCellValue itself subscribes to the state changes, so they trigger the cell's render instead of (or in addition to) the top-level Table.

Our main datagrid example demonstrates this data access via context where it sets the context and then reads from it in renderCellValue.

I've forked your sandbox to show a modified, working demo of this:

  • wrapped the EuiDataGrid usage with DataContext.Provider, providing access to the rows & a quick setState function that can be used to update the rows state
  • took RenderCellValue off the Table class so it is stand-alone, forcing it to use context instead of the shared this.state
  • pull rows and setRows from the context in RenderCellValue

Now when a switch is clicked, it calls setRows from context which calls the Table's setState, triggering Table's render. The render creates a new context object with the updated rows value, and because the context changed it triggers the cell's rendering because they are subscribed to that context. Note that this still causes every cell to re-render, because all they know is the context has updated and not what row was changed. This is where things like MobX can help with extra performance, if needed.

@j-m
Copy link
Contributor Author

j-m commented Oct 27, 2020

Wow! Thank you very much, great answer.

I also have a row-selection checkbox in the grid's leadingControlColumns. The checked state is also managed by the EuiDataGrid.
So why do headerCellRender and rowCellRender work?
As far as I can tell, rowCellRender uses renderCellValue
https://codesandbox.io/s/hopeful-river-9cix2?file=/src/App.tsx

I'll need a high performant state manager for future component anyway, so may as well migrate everything now, which would you recommend?

@chandlerprall
Copy link
Contributor

So why do headerCellRender and rowCellRender work?

Those work in your example because they are re-defined every time in the render function, compared to writing their definition elsewhere and referencing it - e.g. leadingControlColumns={myLeadingControlColumns}.

For performance, you'll need to avoiding changing any of the props listed in #3518 as they cause wider re-renders. Some work on the virtualization (#4170) is shortening that list, but at the moment it should be representative. For testing, I throw a console.log in renderCellValue to check if the cells are re-rendering in response to some action.

For performant state management - both for EuiDataGrid and any larger React application - you'll need something that ties data updates to specific components. MobX does this with observers while Recoil uses atoms to maintain separation. Both of those are good, but are very different to work with so it's up to your preference. There are other state libraries too, just look for the ability to subscribe components to individual pieces of state.

@j-m
Copy link
Contributor Author

j-m commented Oct 28, 2020

Ah, makes sense. Thank you so much for your help! I have a lot of refactoring to do...

@j-m j-m closed this as completed Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants