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

fix: make proxy a no-op when passed a proxy #1038

Closed
wants to merge 1 commit into from

Conversation

aleclarson
Copy link

@aleclarson aleclarson commented Jan 20, 2025

Summary

As far as I can tell, there's no benefit to wrapping a Valtio proxy with proxy(…) but it can happen inadvertently. In this case, Valtio should simply return the proxy unchanged, since it's already subscribable.

In valtio-kit, I'm using proxy(new Foo()) as a compiler hint, so the compiler can know to wrap references to the Foo instance with get(…) inside of watch handlers. In my testing of this compiler hint, I found this undesirable Valtio behavior of “proxying a proxy”.

Check List

  • pnpm run fix:format for formatting code and docs

Copy link

vercel bot commented Jan 20, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
valtio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 20, 2025 7:43pm

Copy link

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.

aleclarson added a commit to aleclarson/valtio-kit that referenced this pull request Jan 20, 2025
This can be reverted when the following PR is merged: pmndrs/valtio#1038
Copy link

pkg-pr-new bot commented Jan 20, 2025

Open in Stackblitz

More templates

npm i https://pkg.pr.new/valtio@1038

commit: 6511f29

@dai-shi
Copy link
Member

dai-shi commented Jan 21, 2025

I'm not sure if I can explain this well, but it feels like there might be a case where we want wrapping proxies again. Adding the proposed check will prevent such a use case, so this isn't acceptable at the moment. Adding the same check with a wrapper is technically possible.

I assume you have your solution on valtio-kit without changing valtio, and this is just to let us know this should be an expected behavior. Thanks for the suggestion anyway.

@dai-shi dai-shi closed this Jan 21, 2025
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

Successfully merging this pull request may close these issues.

2 participants