search

dai-shi / proxy-compare

Compare two objects using accessed properties with Proxy
https://www.npmjs.com/package/proxy-compare
MIT License
283 stars 18 forks source link

fix: reuse target cache for inner createProxy runs #59

Closed Aslemammad closed 1 year ago

Aslemammad commented 1 year ago

Resolves: https://github.com/dai-shi/proxy-memoize/issues/68

This PR tries to attach the target cache weak map so it can be reused later by other inner proxy objects.

From 3s: image (4) To 70ms: image (5)

codesandbox-ci[bot] commented 1 year ago

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.

Latest deployment of this branch, based on commit 46e962cd85b544fa694f33f8314371d64327e7b8:

Sandbox Source
Vanilla Configuration
Vanilla Typescript Configuration
React Configuration
React Typescript Configuration
dai-shi commented 1 year ago

Thanks for the investigation! We removed the weakmap cache in #53, and added to accept the cache in #56. So, my guess is that we don't need to change proxy-compare for now.

Can you do something similar to https://github.com/pmndrs/valtio/pull/664 in proxy-memoize?

Aslemammad commented 1 year ago

Can you do something similar to https://github.com/pmndrs/valtio/pull/664 in proxy-memoize?

I tried that by making configurable: true, but wasn't successful. in case you know how to do it, let me know. While I was debugging the cause, I found out targetCache wasn't being used in the next runs of createProxy, so I passed it there and it worked (I also removed the target copy cache from the first commits).

dai-shi commented 1 year ago

I tried that by making configurable: true

I think I misunderstand the problem. So, for https://github.com/dai-shi/proxy-memoize/issues/68 case, needsToCopyTargetObject returns false and what is slow is needsToCopyTargetObject call itself?

dai-shi commented 1 year ago

Can you do something similar to pmndrs/valtio#664 in proxy-memoize?

I meant something like this: https://github.com/dai-shi/proxy-memoize/pull/70

Aslemammad commented 1 year ago

what is slow is needsToCopyTargetObject call itself?

Yes, that's the slow part.

I meant something like this: https://github.com/dai-shi/proxy-memoize/pull/70

I used this solution, and it works if we use the current solution i added now in proxy-compare (reusing the target cache), without that, it won't work.

dai-shi commented 1 year ago

I meant something like this: dai-shi/proxy-memoize#70

I used this solution, and it works if we use the current solution i added now in proxy-compare (reusing the target cache), without that, it won't work.

Now, I get your point. Yes, this PR now seems a valid improvement. Can you update the PR title and the description?