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

splitAtom(arrayAtom) returns inconsistent length temporarily with multiple renderers since v1.3.3 #728

Closed
sxy8469 opened this issue Sep 22, 2021 · 15 comments
Labels
wontfix This will not be worked on

Comments

@sxy8469
Copy link

sxy8469 commented Sep 22, 2021

const itemsAtom = atom([])
const itemAtomsAtom = splitAtom(itemsAtom)

const items= useAtomValue(itemsAtom );
const itemAtoms= useAtomValue(itemAtomsAtom);
items.length ≠ itemAtoms.length ???

@dai-shi
Copy link
Member

dai-shi commented Sep 22, 2021

Thanks for reporting. This seems unexpected.
Would you be able to create a minimal reproduction in codesandbox?

It works in v1.3.2, but not v1.3.3, right?
Either #678 or #689 might cause an issue. 🤔

@sxy8469
Copy link
Author

sxy8469 commented Sep 23, 2021

Thanks for reporting. This seems unexpected.
Would you be able to create a minimal reproduction in codesandbox?

It works in v1.3.2, but not v1.3.3, right?
Either #678 or #689 might cause an issue. 🤔

https://codesandbox.io/s/react-typescript-forked-gi301?file=/src/App.tsx

@dai-shi
Copy link
Member

dai-shi commented Sep 23, 2021

It works in v1.3.2, but not v1.3.3, right?
Either #678 or #689 might cause an issue.

Either #687 or #689, and it's #687 causing this.

@dai-shi
Copy link
Member

dai-shi commented Sep 23, 2021

Hm, removePoint removes the original array. A bit of surprising it's working pre v1.3.3.
Let me see if I can repro in tests.

@dai-shi
Copy link
Member

dai-shi commented Sep 23, 2021

Okay, I found the issue is not reproducible if we don't use react-konva.

dai-shi added a commit that referenced this issue Sep 23, 2021
@dai-shi
Copy link
Member

dai-shi commented Sep 23, 2021

Please see #729. I assumed react-konva triggers action asynchronously, and the error is reproduced. And it is solved with unstabled_batchedUpdates.

However, I was able to only solve your codesandbox with batchedUpdates + setTimeout. So, my assumption can be wrong.

Do you think you can reproduce the issue without react-konva and/or can you modify the test in #729 to make it fail like with react-konva (even with batchedUpdates)?

@sxy8469
Copy link
Author

sxy8469 commented Sep 26, 2021

Okay, I found the issue is not reproducible if we don't use react-konva.

Yes, but the "react-konva" is required.
I still hope this problem can be solved in the next version

@dai-shi
Copy link
Member

dai-shi commented Sep 26, 2021

But, we don't know what is the problem, thus we can't investigate a possible solution. We would like to fix it for sure. Do you happen to know how react-konva works? Or, can you try to reproduce the issue without react-konva?
Meanwhile, this is a workaround: https://codesandbox.io/s/react-typescript-forked-6dqs8

@sxy8469
Copy link
Author

sxy8469 commented Sep 27, 2021

But, we don't know what is the problem, thus we can't investigate a possible solution. We would like to fix it for sure. Do you happen to know how react-konva works? Or, can you try to reproduce the issue without react-konva?
Meanwhile, this is a workaround: https://codesandbox.io/s/react-typescript-forked-6dqs8

Thanks, I temporarily reverted to 1.3.2 version : )

@sxy8469
Copy link
Author

sxy8469 commented Sep 27, 2021

This problem also occurs when using "@inlet/react-pixi".
It should be caused by using "ReactFiberReconciler"

Example:
https://codesandbox.io/s/react-typescript-forked-wwyru?file=/src/App.tsx

@dai-shi
Copy link
Member

dai-shi commented Sep 27, 2021

We need somebody to help on this.
If anyone is interested or got challenged, please try to create a reproduction of the issue without react-konva.

@dai-shi dai-shi added the help wanted Please someone help on this label Sep 27, 2021
@dai-shi dai-shi changed the title 1.3.5 useAtomValue(splitAtom(arrayAtom)) bug, 1.3.2 is right splitAtom(arrayAtom) returns inconsistent length temporarily with react-konva since v1.3.3 Sep 27, 2021
@dai-shi
Copy link
Member

dai-shi commented Sep 27, 2021

This problem also occurs when using "@inlet/react-pixi".
It should be caused by using "ReactFiberReconciler"

Oh, that's good to know! Then, I guess it would be extremely hard to solve this now.
But, the good news is when React 18 comes, it will probably batch actions, so hopefully it will work.

react-three-fiber from @pmndrs uses a separate reconciler too.

@dai-shi dai-shi changed the title splitAtom(arrayAtom) returns inconsistent length temporarily with react-konva since v1.3.3 splitAtom(arrayAtom) returns inconsistent length temporarily with multiple roots since v1.3.3 Sep 27, 2021
@dai-shi
Copy link
Member

dai-shi commented Sep 27, 2021

So, given that this is not solvable, my suggestion is to use (read) only the result of splitAtom, not mixing with the original arrayAtom (of course, we need to mutate arrayAtom).

Here's the fix: https://codesandbox.io/s/react-typescript-forked-6u6pe?file=/src/App.tsx

Hope it helps.

@dai-shi dai-shi removed the help wanted Please someone help on this label Sep 27, 2021
@dai-shi dai-shi changed the title splitAtom(arrayAtom) returns inconsistent length temporarily with multiple roots since v1.3.3 splitAtom(arrayAtom) returns inconsistent length temporarily with multiple renderers since v1.3.3 Sep 27, 2021
@dai-shi dai-shi added the on hold Nothing is actionable now, but hope to revisit in the future label Nov 9, 2021
@dai-shi
Copy link
Member

dai-shi commented Nov 30, 2021

@Thisen We might be interested if #854 changes the behavior. Otherwise, we can close this as wontfix. Or, you can dig into it. I'm happy if you rebuild splitAtom from scratch.

@dai-shi
Copy link
Member

dai-shi commented Dec 30, 2021

Unfortunately, v1.5.0 doesn't change the behavior. Please use the workaround.
Let me close this as wontfix. That doesn't mean to stop someone to challenge it.

@dai-shi dai-shi closed this as completed Dec 30, 2021
@dai-shi dai-shi added wontfix This will not be worked on and removed on hold Nothing is actionable now, but hope to revisit in the future labels Dec 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants