search

paol-imi / muuri-react

The layout engine for React
https://paol-imi.github.io/muuri-react/
MIT License
359 stars 52 forks source link

Cannot read property 'key' of null #26

Closed tschneid closed 4 years ago

tschneid commented 4 years ago

In my scenario, I dynamically create grids (multiple MuuriComponents) using some state of my items. My items have a button to change the state, so in the UI they should immediately be moved to another grid by pressing this button (no dragging involved).

Most of the time this throws an Uncaught TypeError: Cannot read property 'key' of null since item._component is null in handleArray coming from GridComponent.

Since this does not always happen, I guess there is some timing problem, in which the item is not properly removed from the old grid (the item is still present, but its component is null). Maybe it's related that vars.itemsToRemove is [].

Do you have any ideas on how to resolve that?

paol-imi commented 4 years ago

Maybe it's related that vars.itemsToRemove is [].

In the current implementation the items to remove are only those whose component has been unmounted, so an empty array should be right.

How do you send items from one grid to another?

(Before looking further, check if the keys you use for the items are unique, this error could be given by that).

tschneid commented 4 years ago

How do you send items from one grid to another?

I'm not explicitly sending them to another grid. An item can change its state (from anywhere in the application), which logically makes it belong to another grid. So the passed children are updated.

(Before looking further, check if the keys you use for the items are unique, this error could be given by that).

I'm positive that the keys are unique (as they are unique keys from a database).

I've just checked whether there is a short period in which the moved item appears in multiple grids (which would explain a non-unique key), but that does not seem to be the case. The passed children are correctly updated (the item is removed from the children of the sender grid and added to the children of the receiving grid).

I can try to replicate the problem in a Codesandbox by next week.

paol-imi commented 4 years ago

So if I understand correctly you are doing something like this

<MuuriComponent key="A">
  <Item key="1" />
  <Item key="2" />
</MuuriComponent>

<MuuriComponent key="B">
  <Item key="3" />
</MuuriComponent>
rerenderTheApp();
<MuuriComponent key="A">
  <Item key="1" />
</MuuriComponent>

<MuuriComponent key="B">
  <Item key="2" />
  <Item key="3" />
</MuuriComponent>

Without using manual reparenting and having the old Item unmounted and the new one re-mounted.
(Reparenting is automated only after a drag).

Can you confirm that this is the case?

If it is, itemsToRemove = [] would be a problem.

I can try to replicate the problem in a Codesandbox by next week.

If you can load it, I'll take a look.

tschneid commented 4 years ago

Can you confirm that this is the case?

Yes, that's the case (since the underlying data can potentially change from anywhere, not just by dragging the elements around).

I could reproduce the error here: https://codesandbox.io/s/muuri-react-kanban-lplcx

This is the original Kanban demo. I just added the button and the sort prop for each MuuriComponent.

paol-imi commented 4 years ago

I will investigate further as soon as I have free time, but it may not be in the short term. For now you can try applying reparenting as I do in tests.

Temporarily simulate drag

const item = grid.getItems().find(item => item.getKey() === "4")
// Temporary replacement
const isDragging = item.isDragging
item.isDragging = () => true

and then manually submit the item before re-rendering

grid.send(item, toGrid, 0)

You can also use muuriMap for simplicity.

const grid = muuriMap.get("Todo");
paol-imi commented 4 years ago

I did some tests and the problem does not seem to affect the manual reparenting, so I made some simple changes to the kanban demo and added buttons on each item to simulate the functioning of your app.

https://codesandbox.io/s/muuri-react-kanban-tm4w6?file=/index.js

It could be a good place to start.

I will try to get as soon as possible to look for and solve the bug you have encountered, however, this approach should be the "best" one, as the item sent maintains its state and is not re-mounted.

tschneid commented 4 years ago

Did you have a chance to look into this?

To make it a bit more tangible, in my app I have "projects" that can be open or closed. "Open" and "closed" are shown in different Muuri grids. To close a project, I can drag it to the "closed" grid or press a button.

Interestingly, I only see this error, when the exact same project was dragged to another grid before. So initially, pressing the button works (repeatedly).

Do you have any pointers where I could start to have a look in the muuri-react code to tackle this issue?

paol-imi commented 4 years ago

I'm planning a package refactoring to significantly improve the codebase and bring some fixes.

Unfortunately, university exams are in full swing, and the short time I have available I am using it to design this refactoring (I am also dividing some parts of the code in external packages). So now I have a hard time managing fixes.

By the time I finish my exams, work on this project will go much faster.

If you want to take a look, I'll tell you where I think the problem lies.

I believe that the instance is not removed from the grid, but it has been deprived of the decoration (which is now null), so in subsequent renderings, access the decoration throws the error.

paol-imi commented 4 years ago

I could reproduce the error here: https://codesandbox.io/s/muuri-react-kanban-lplcx

I just released some fixes and tried your example on codesandbox with the updated package.

From the first tests it seems that the problem is no longer present (at least in the example). Let me know if the problem persists or has been resolved.

tschneid commented 4 years ago

Awesome, seems to work! Thanks @Paol-imi :+1: