fix: frozen afterward with targetCache #71
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
StepanDyubin
force-pushed
the
fix/frozen-with-target-cache
branch
from
57aef8b to
f7cdc5d
Compare
There was a problem hiding this comment.
Thanks for the suggestion, but I don't think this is acceptable as-is.
We need to prove needsToCopyTargetObject isn't costly.
Even if we proved it (which we probably could), it would be still more performant without it, so if we care the performance, we should keep the current algorithm.
(We might be able to remove storing target in the target cache though.)
| const target = getOriginalObject(obj); | ||
| if (needsToCopyTargetObject(target)) { |
There was a problem hiding this comment.
So, previously, we can avoid these computations with the target cache.
I think getOriginalObject is fairly fast, but needsToCopyTargetObject has a loop.
So, there's a performance drawback. Since the target cache is introduced to improve the performance in Valtio, it has to be considered.
| const targetAndCopied = | ||
| targetCache && (targetCache as TargetCache<typeof obj>).get(obj); |
There was a problem hiding this comment.
In this usage, we don't need to store the target in the cache.
| targetAndCopied = [target, copyTargetObject(target)]; | ||
|
|
||
| const target = getOriginalObject(obj); | ||
| let copiedTarget; |
There was a problem hiding this comment.
| let copiedTarget; | |
| let copiedTarget: typeof target | undefined; |
|
Closing as stale. |
Resolves #70