proxy set handler returned false for property "'toast'"

[M0hammedImran]

export interface IStore {
  toast: ToastProps;
  resetToast: () => void;
}

export const store = proxy<IStore>({
  toast: {
    success: null,
    error: null,
    open: false,
  },
  resetToast() {
    this.toast = {
      success: null,
      error: null,
      open: false,
    };
  },
});

[dai-shi]

Hey, thanks for trying the library!

this is hard. It doesn't point to an object you are expecting. Would you please try the following?

export const store = proxy<IStore>({
  toast: {
    success: null,
    error: null,
    open: false,
  },
  resetToast: () => {
    store.toast = {
      success: null,
      error: null,
      open: false,
    };
  },
});

[dai-shi]

This is not the first time the error message confuses people. #178

I understand the problem is serious.
To my knowledge, it's hard to detect this at runtime.
But, we don't need to give up. Let's try two challenges.

1. explore if we can customize the error message (this requires deeper understanding in valtio and proxy-compare)
2. add a new rule to warn this usage in eslint-plugin-valtio. Though this is valid for some use cases, we can't detect good one and bad one (would be nice if it could).

I'd like to ask for volunteers.
cc @Aslemammad @YPAzevedo @TheEhsanSarshar @israelidanny @a-eid @barelyhuman @lxsmnsyc

[barelyhuman]

I wouldn't mind going through both the recommendations and let you know what my thinking of this is

[etodanik]

@dai-shi I wouldn't mind playing around with option (1). I think it's a better approach than Option (2) personally both in ease of implementation and practicality (we can't guarantee that someone is running a great & fully functioning linting config)

I'll get back with feedback in the next day or two if nobody gets there before me.

[dai-shi]

@israelidanny I gave up option 1 in a nice way, so it might require some tricks. One thing to note is we don't want to modify proxy-compare for valtio-only problems.

@barelyhuman So, would you primarily take option 2?

I appreciate your helps. Please also refer discussions in #178.

[M0hammedImran]

This was confusing to say the least. The error was pointing to the store. But it was my fault.
I was trying to call resetToast on snap

const snap = useSnapshot(store);
snap.resetToast();

After that fix. It's been working great.

[dai-shi]

If one prefers using this, they need to make sure calling the method on store not snap.

store.resetToast();

Another discussion in #205.

[M0hammedImran]

If one prefers using this, they need to make sure calling the method on store not snap.

store.resetToast();

Another discussion in #205.

I made the change. Now I'm using store for mutation and snap for reading and reacting to changes.

[etodanik]

There is the option of binding all the methods to the correct this while building the snapshot, but it may not be the approach we want here. This way there wouldn't be any error, but it does introduce a little bit of "magic". What do you think @dai-shi @barelyhuman ?

[barelyhuman]

There is the option of binding all the methods to the correct this while building the snapshot, but it may not be the approach we want here. This way there wouldn't be any error, but it does introduce a little bit of "magic". What do you think @dai-shi @barelyhuman ?

While that would help with the developer experience but then we start assuming what the developer wants, someone might use this inside a function and expect this to have a different context, plus the support would then have to be handled for all types of function definitions.

As in, context of this for arrow is going to be different and context for for lamda, named, object method is going to different, I think we'd over engineer it and then call for more problems, I still have to do the above while adding to the linter rules but that is to warn the user of the keyword, and not assume the context of this so it's still a viable solution.

There can definitely be better solutions , i'm open to any. Since I've only written like 2-3 lines on the eslint plugin as of now

[dai-shi]

There is the option of binding all the methods to the correct this while building the snapshot, but it may not be the approach we want here. This way there wouldn't be any error, but it does introduce a little bit of "magic". What do you think @dai-shi @barelyhuman ?

The current behavior is intentional to make it as pure JS as possible.

this on snapshot is useful for getValue function.

const state = proxy({
  count: 1,
  getDoubledCount() {
    return this.count * 2
  },
})

console.log(state.getDoubledCount())

const Component = () => {
  const snap = useSnapshot(state)
  return <div>{snap.getDoubledCount()}</div>
}

This doesn't work if we bind getDobuledCount().

[dai-shi]

option 2 is merged. So, let me close this.

@israelidanny Let us know if you are still working on this.