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

Different behavior for list rendered by normal react and react-konva. #636

Closed
denbezrukov opened this issue Dec 14, 2021 · 4 comments
Closed

Comments

@denbezrukov
Copy link

Hi,
Thank you for your library, it's awesome!

I'm trying to understand why react-konva has different behavior with list.
I have a test application
It uses react, react-redux, react-konva.

export default function App() {
  return (
    <div className="App">
      <Stage width={300} height={100}>
        <Provider store={store}>
          <Layer>
            <KonvaList />
          </Layer>
        </Provider>
      </Stage>
      <Provider store={store}>
        <DomList />
      </Provider>
    </div>
  );
}

It renders list with react-konva:

export function KonvaList() {
  const list = useAppSelector((state) => state.list.list);
  return (
    <>
      {list.map((v) => {
        return <KonvaItem key={v.id} id={v.id} />;
      })}
    </>
  );
}
function KonvaItemComponent(props: { id: number }) {
  const dispatch = useAppDispatch();

  const storeItem = useAppSelector((state) => {
    const item = state.list.list.find((value) => value.id === props.id);
    const { id, value } = item!;
    return {
      id,
      value
    };
  });

  return (
    <Rect
      x={60 * storeItem.id}
      y={20}
      width={50}
      height={50}
      fill={"black"}
      shadowBlur={5}
      onClick={() => dispatch(remove(props))}
    />
  );
}

It renders list with react:

export function DomList() {
  const list = useAppSelector((state) => state.list.list);
  return (
    <div>
      {list.map((v) => {
        return <DomItem id={v.id} key={v.id} />;
      })}
    </div>
  );
}
function DomItemComponent(props: { id: number }) {
  const dispatch = useAppDispatch();

  const itemStore = useAppSelector((state) => {
    const item = state.list.list.find((value) => value.id === props.id);
    const { id, value } = item!;
    return {
      id,
      value
    };
  });
  return <div onClick={() => dispatch(remove(props))}>{itemStore.id}</div>;
}

If we click on rectangle or on number it deletes clicked item from store.
Problem with list which is rendered by react-konva. It has an error if we delete an item. But there is no any error with normal list.
You can comment KonvaList or DomList in App.tsx.

@lavrton
Copy link
Member

lavrton commented Dec 14, 2021

It looks like it is because of order of updates is not correct. The child component is updated before the parent component.

It comes from react-redux. I am not sure I can do anything from react-konva side.

See this for more info:
reduxjs/react-redux#1589
reduxjs/react-redux#1510

@denbezrukov
Copy link
Author

denbezrukov commented Dec 14, 2021

Hmm,
If it was react-redux problem it would reproduce with normal react components. But if we comment KonvaList usage in App.tsx and delete item, we don't get any error.

@denbezrukov
Copy link
Author

denbezrukov commented Dec 14, 2021

If I understand correctly, child component updating before parent component is valid case. They catch error on store update and wait error on next component render.

Following action sequence:

  1. Dispatch delete action.
  2. Store gets new state.
  3. Store starts notify current subscribers.
  4. Children subscribe callback is called before parent, we catch error, call force render and wait next child render with error.
  5. Parent gets update.
  6. Parent unmount deleted children component.
  7. Children component render, which got error on step 4, isn`t called because component was unmounted.

But with react-konva it seems that we call children render on step 7.

@lavrton
Copy link
Member

lavrton commented Apr 27, 2022

Closing the issue because I am not sure what to fix here.

If it is still important for you, please make a minimal demo to reproduce the issue.

@lavrton lavrton closed this as completed Apr 27, 2022
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