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

valtio should avoid state change for compare equal value #511

Closed
wenerme opened this issue Jul 30, 2022 · 10 comments
Closed

valtio should avoid state change for compare equal value #511

wenerme opened this issue Jul 30, 2022 · 10 comments

Comments

@wenerme
Copy link

wenerme commented Jul 30, 2022

new to valtio, I expected mutate state is like immer with proxy, if assign the same value, the state should not change, but seems not the case.

like

const state = proxy({
  ref: ref(0),
  object: {}
});

// should prevent the rerender if the value is reference equal
state.ref = ref(0)

// should avoid change
state.object = {}

Is it possible to pass the custom comparator or use with immer to prevent the change ?

@wenerme wenerme changed the title assign ref should do a reference compare valtio should avoid state change for compare equal value ? Jul 30, 2022
@wenerme wenerme changed the title valtio should avoid state change for compare equal value ? valtio should avoid state change for compare equal value Jul 30, 2022
@dai-shi
Copy link
Member

dai-shi commented Jul 31, 2022

It should skip setting if it's the same as the previous one. (Compared with Object.is by default.)

For example, this should work as expected.

const state = proxy({ obj: {} })
state.obj = state.obj

But, this doesn't,

state.obj = {}

because it's a new object. This should be the default behavior that we shouldn't change.

@dai-shi
Copy link
Member

dai-shi commented Jul 31, 2022

state.ref = ref(0)

I don't feel like writing like this, but it should just mean state.ref = 0, which skips changes, no?

@wenerme
Copy link
Author

wenerme commented Aug 1, 2022

Thanks, I expected immer like mutation detect, seems I was wrong, but do hope valtio support custom change compare, valtio already batched mutation.

state.ref = ref(0)

Just use 0 as a reference, due to #510 , the 0 stand for <Comp/> or document.body like value, should use Object.is compare the ref value changed or not to avoid the notify.

@dai-shi
Copy link
Member

dai-shi commented Aug 1, 2022

I expect it work fine with document.body.

state.ref = ref(document.body)
state.ref = ref(document.body) // should be no-op

@dai-shi
Copy link
Member

dai-shi commented Aug 1, 2022

btw, we could create a proxy object in advance.

const obj = proxy({ foo: 1 }) // initialize object outside
const state = proxy({ obj })
state.obj = obj // should be no-op

@dai-shi
Copy link
Member

dai-shi commented Aug 1, 2022

I expected immer like mutation detect, seems I was wrong

As I wrote in a different issue/discussion, valtio is designed like an opposite of immer.

but do hope valtio support custom change compare, valtio already batched mutation.

We do want to avoid adding features in valtio. It's one of the ground rules from the start.
If you need a custom comparator, it should be nice if we can make it possible to it on your end. There's an experimental feature called unstable_getHandler, with which you can specify a function instead of Object.is.

is: Object.is,

But this feature is not very developer friendly. It's basically for 3rd-party library developers. It means you should be able to create proxyWithCustomComparator util.

@wenerme
Copy link
Author

wenerme commented Aug 1, 2022

Thanks, I will look into it.

@wenerme
Copy link
Author

wenerme commented Aug 1, 2022

Wow, after

import eq from "react-fast-compare";
unstable_getHandler(state).is = eq

valtio works like expected, thanks.

https://codesandbox.io/s/valtio-render-test-lzqhf7

Can close the issue now.

@dai-shi
Copy link
Member

dai-shi commented Aug 30, 2022

@wenerme When #528 lands, your solution will no longer work, but it will expose a new powerful function, with which you can customize is.

@wenerme
Copy link
Author

wenerme commented Aug 30, 2022

@dai-shi thanks, will watch the release

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