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

False positive when proxy object is nested inside a regular object #38

Open
nmay231 opened this issue Dec 20, 2022 · 3 comments
Open

False positive when proxy object is nested inside a regular object #38

nmay231 opened this issue Dec 20, 2022 · 3 comments

Comments

@nmay231
Copy link

nmay231 commented Dec 20, 2022

When you use useSnapshot(nestedState.proxy) when nestedState is a regular object, you get a false positive of Mutating a proxy object itself. this might not be expected as it's not reactive. eslint(valtio/state-snapshot-rule).

Having just nestedState.proxy.settings = {}; is not enough for whatever reason. It has to be assigned to a second proxy object. nestedState.proxy.settings = state2.settings;

import { proxy, useSnapshot } from "valtio";
import { useEffect } from "react";

const nestedState = { proxy: proxy({ settings: null as null | Record<string, unknown> }) };
const state2 = proxy({ settings: null as null | Record<string, unknown> });

export function Component() {
    // Bad
    const snap = useSnapshot(nestedState.proxy);

    // Good
    // const proxy = nestedState.proxy;
    // const snap = useSnapshot(proxy);

    // This is where the linting error occurs
    nestedState.proxy.settings = state2.settings;

    // This is a more realistic example.
    // useEffect(() => {
    //     nestedState.proxy.settings = state2.settings;
    // }, []);

    return <div>{JSON.stringify(snap.settings)}</div>;
}
@dai-shi
Copy link
Member

dai-shi commented Dec 20, 2022

Yeah, sounds like a false positive. Not sure if it's solvable. @barelyhuman ?

@barelyhuman
Copy link
Collaborator

I'll check the code but from what i've worked with till now, I'm sure that the proxy identifier is not detectable in this case.

To be fair, it's not right or wrong but what's implemented right now and since it's obvious that it could be more than just a single identifier the same should be implemented.

Not a small change though so might take a bit long.
For now you can just alias and move forward with it

const proxy = nestedState

2nd problem is that because of how the initial implementation works, we don't know anymore if nestedState is the proxy or nestedState.proxy is the proxy so the linter can't really work with the 2nd level state change and will give an error.

The functionality will work as is, it's just that the linter plugin has no idea of the identifier.
and if you **use the alias to modify, the linter should not complain about it

@nmay231
Copy link
Author

nmay231 commented Dec 24, 2022

Gotcha.

FYI, it doesn't have to be a second proxy state as the assignment, any property of a regular object will do.

const nested = {object: null}

// This is where the linting error occurs
nestedState.proxy.settings = nested.object;

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

3 participants