pmndrs / eslint-plugin-valtio Goto Github PK

Lets say i have a hook file with a state in it. useAppState.ts

export const state = proxy({
  isLocked: false,
});

export const useAppState = () => {
  const snap = useSnapshot(state);
  return {
    ...snap,
    state,
  };
};

File 2 SomeComponent.tsx

const { state, isLocked } = useAppState();

const handleClick = () => {
   console.log(isLocked); // No warning
};

Hopefully this can be resolved also, as this will probably lead to hard to catch bugs.

Environment

• eslint-plugin-valtio - ^0.6.1

Description

It is common to memoize a value that depends upon some snapshots and other properties. but when using snapshot in useMemo, eslint reports **Better to just use proxy state", just like below:

Or maybe I am using an anti-pattern to valtio? the problemic line is at https://github.com/xiechao06/valtio-groups/blob/5001116213ed2a08a5ed4a8fc447e29cf7c5e6f1/src/App.tsx#L33

Best regards!

Follow-up of this discussion

Linter complains when you mutate a snap in a custom hook (not within a React component) to use it in a component.

const state = proxy({ count: 1 })

const useDoubled = () => {
  const snap = useSnapshot(state)
  return snap.count * 2
                ^^^^ Better to just use proxy state.eslint(valtio/state-snapshot-rule)
}

const Component = ()=> {
   const doubled = useDoubled()
  return <>{doubled}
}

Other example where linter complains and it shouldn't:

const Component = () => {
  const snap = useSnapshot(store)
 const double = snap*2
                 ^^^^ Better to just use proxy state.eslint(valtio/state-snapshot-rule)
 return <>{double}</>
} 

This code gives a warning Better to just use proxy state.eslint(valtio/state-snapshot-rule)

const MyComponent = () => {
  const snap = useSnapshot(state);
  return <div>{snap.count}</div>;
};

It happens only when declare a component using an arrow function.
No warning when declare using function keyword.

I guess it's because the function component is detected as a callback.

In v2, the incompatibility with useMemo and so on will be resolved, pmndrs/valtio#866
so, this plugin should avoid warning such usages with v2.

I wonder if there's a way in eslint to change the behavior based on Valtio versions. Otherwise, we can simply upgrade, which is reasonable for simplicity too.

@barelyhuman Can you work on it? It's not in a hurry.

Getting an error `TypeError: Cannot read properties of null (reading 'parent').

The culprit is the following line.

I believe varDefOfFunc can be null since getParentOfNodeType can return null.

valtio v0.8.0 uses a new hook name useSnapshot.

I literally coped the example. No eslint warning shows. I'm pretty sure my valtio eslint config is working, since if i write state.count in render, it will show a warning from (valtio/state-snapshot-rule).

const state = proxy({ count: 0 })

function App() {
  const snap = useSnapshot(state)
  const handleClick = () => {
    console.log(snap.count) // <- No warning shows here
  }
  return (
    <div>
      {snap.count} <button onClick={handleClick}>click</button> 
    </div>
  )
}

If the user creates a computed field deriving value from another computed, the computed source should be declared first:

pmndrs/valtio#84

From pmndrs/valtio#76 (comment)

let state = proxy(['hello'])

  // in component
  const snap = useProxy(state)
  const handleClick = () => {
    state = ['hello', 'world']
  }

const state = proxy({ arr: ['hello'] })

  // in component
  const snap = useProxy(state.arr)
  const handleClick = () => {
    state.arr = ['hello', 'world']
  }

These are patterns that behave unexpectedly.

Hi, I have been using valtio for several months now and decided to try using this plugin today and this is what I ran into.

Code:

class ActionDispatcher {
    modalRenderer: IModalRenderer | null = null;

    openModal(modal: ModalComponent, customData?: unknown): Promise<void> {
        return new Promise<void>((resolve, reject) => {
            // FIXME: error is here
            if (this.modalRenderer) { 
                this.modalRenderer(modal, resolve, customData);
            }
        });
    }
}

Error:

ESLint: 7.32.0

TypeError: Cannot read properties of null (reading 'id')
Occurred while linting /myproject/src/actions/ActionDispatcher.tsx:15
    at ThisExpression (/node_modules/eslint-plugin-valtio/index.js:366:30)
    at /node_modules/eslint/lib/linter/safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (/node_modules/eslint/lib/linter/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (/node_modules/eslint/lib/linter/node-event-generator.js:293:26)
    at NodeEventGenerator.applySelectors (/node_modules/eslint/lib/linter/node-event-generator.js:322:22)
    at NodeEventGenerator.enterNode (/node_modules/eslint/lib/linter/node-event-generator.js:336:14)
    at CodePathAnalyzer.enterNode (/node_modules/eslint/lib/linter/code-path-analysis/code-path-analyzer.js:711:23)
    at /node_modules/eslint/lib/linter/linter.js:960:32
    at Array.forEach (<anonymous>)

Here's a slightly sanitized stacktrace:

$ eslint . --ext .js,.jsx,.ts,.tsx --color

Oops! Something went wrong! :(

ESLint: 8.15.0

TypeError: Cannot read properties of undefined (reading 'type')
Occurred while linting /Users/jason/my_app/js/hooks/useDragElement.ts:45
Rule: "valtio/state-snapshot-rule"
    at /Users/jason/my_app/node_modules/eslint-plugin-valtio/index.js:383:36
    at Array.forEach (<anonymous>)
    at isUsedInUseProxy (/Users/jason/my_app/node_modules/eslint-plugin-valtio/index.js:374:19)
    at isUsedInUseProxy (/Users/jason/my_app/node_modules/eslint-plugin-valtio/index.js:388:47)
    at Identifier (/Users/jason/my_app/node_modules/eslint-plugin-valtio/index.js:200:225)
    at ruleErrorHandler (/Users/jason/my_app/node_modules/eslint/lib/linter/linter.js:1114:28)
    at /Users/jason/my_app/node_modules/eslint/lib/linter/safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (/Users/jason/my_app/node_modules/eslint/lib/linter/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (/Users/jason/my_app/node_modules/eslint/lib/linter/node-event-generator.js:297:26)

I'm not quite sure how to create a code sandbox that reproduces the error because I don't see a way to run eslint in the code sandbox, but here's a codesandbox that has the bits of the file that seem most relevant:
https://codesandbox.io/s/nifty-sound-thwx6r?file=/src/lib/useDrag.ts

Add explanation of why this plugin is useful / examples of the cases it lints against. It's clear by looking at the source but would be helpful to include in readme

Somewhere (there #16 (comment)), we discussed the limitation of linting because we can only analyze code statically.
If we could use type information, we could make linting better.
I stumbled upon eslint-plugin-functional which utilizes types.

We should be able to detect this:

const state = proxy({ obj: {}, num: 1 })

const Component = () => {
  const { obj, num } = useSnapshot(state)

  useEffect(() => {
    // ...
  }, [obj]) // this is not allowed

  useEffect(() => {
    // ...
  }, [num]) // but this is fine

  return "wow"
}

Related: pmndrs/valtio#633 (comment)

cc: @barelyhuman @sarimarton

Noticed this when installing and though I'd let you know...

When you add 'plugin:valtio/recommended', to the extends array it enables all the recommended rules from the plugin. Adding the rules directly (second step in the instructions) is redundant. Specific rule entries are only needed if you want to do something different than the recommended.

e.g. - user only wants "valtio/avoid-this-in-proxy": ["warn"], so they add that to the rules and do not add the recommended settings to extend.

Hello,
I'm facing an issue with the latest version of eslint-plugin-valtio (0.4.4). The problem is that I'm using useSnapShot to re render the variable from the Store.
When I replace this implementation and call the variable directly from the Store the warning get fixed but the variable doesn't render anymore...

I tried changing the declaration of arrow function for the keyword function without success (#22).

const { isEditedMInformation } = useSnapshot(memberInformationStore)
vs
memberInformationStore.isEditedMInformation

I wasn't able to find any solution to my issue. Can somebody give me a guidance about it? Thanks!

Hello, thanks for making this plugin, it's been helpful while learning valtio!

I believe I've found a bug 😅

In the following example, the goal is to get the useExampleX hook's useEffect callbacks to execute when one of the watched properties changes, and without a parent component passing in a new version of state. An additional goal is that only when a1's properties change would the hook rerender.

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

const state = proxy({
  a0: { b: { c: { d: 'hello', e: 'world' } } },
  a1: { b: { c: 'a1c' } },
});

// executes ok, lint notok
function useExample0(s: typeof state) {
  const snap = useSnapshot(s.a1);

  useEffect(() => {
    if (snap.b.c === 'a1c') {
      state.a1.b.c = 'example';
    }
  }, [snap.b]);
}

// executes notok, lint ok: this is what blindly following the linter
// recommendation would produce!
function useExample1(s: typeof state) {
  useEffect(() => {
    if (s.a1.b.c === 'a1c') {
      state.a1.b.c = 'example';
    }
  }, [s.a1.b.c]);
}

// executes ok, lint notok
function useExample2(s: typeof state) {
  const {b: {c} } = useSnapshot(s.a1);

  useEffect(() => {
    if (c === 'a1c') {
      state.a1.b.c = 'example';
    }
  }, [c]);
}

// executes ok, lint ok (but renders for a0 changes as well)
function useExample3(s: typeof state) {
  const snap = useSnapshot(s);
  const {c} = snap.a1.b;

  useEffect(() => {
    if (c === 'a1c') {
      state.a1.b.c = 'example';
    }
  }, [c]);
}

// executes ok, lint ok
function useExample4(s: typeof state) {
  const snap = useSnapshot(s.a1);
  const {c} = snap.b;

  useEffect(() => {
    if (c === 'a1c') {
      state.a1.b.c = 'example';
    }
  }, [c]);
}

// executes ok, lint ok
function useExample5(s: typeof state) {
  const snap = useSnapshot(s.a1).b;
  const {c} = snap;

  useEffect(() => {
    if (c === 'a1c') {
      state.a1.b.c = 'example';
    }
  }, [c]);
}

// executes ok, lint ok
function useExample6(s: typeof state) {
  const c = useSnapshot(s.a1).b.c;

  useEffect(() => {
    if (c === 'a1c') {
      state.a1.b.c = 'example';
    }
  }, [c]);
}

It's unclear exactly what is happening. The bug seems to be triggered by destructuring directly from useSnapshot or directly referencing the snapshot in the useEffect. Or is it that assignment of the snapshot defeats the lint rule?

A worrying aspect is that if a developer blindly followed the recommendation from the linter, it would produce code that does not behave as expected. It's somewhat obvious in these contrived examples (useExample1), but is less obvious in a larger codebase with multiple layers of hooks.

A workaround for now appears to be:

• useSnapshot(state.a1) on the parent you need
• destructure or assign child properties in a separate statement

 const { project } = useSnapshot(ProjectStore);

<Select size="small" value={project.lang} onChange={onLanguageSelect}>
    {Object.keys(project.langData).map((language: string) => (               <--- Error line
        <Select.Option key={language} value={language}>
            {Languages.find((x) => x.code === language)?.name}
        </Select.Option>
     ))}
</Select>

ESLint: 8.21.0

TypeError: Cannot read properties of undefined (reading 'endsWith')
Rule: "valtio/state-snapshot-rule"

Snapshots are made to be used with react, so we recommend them for mutating, and not states directly.

const state = proxy({ count: 0})

function App() {
  const handleClick = () => {
    state.count = state.count + 1 // Mutating a proxy object itself. this might not be expected as it's not reactive.
  }
 return <button onClick={handleClick}>mutate</button> 
}

What's the recommended way of mutating a snapshot? Thanks.

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>;
}

I get the following warning on the snap variable in the condition when trying to use the snapshot to conditionally render some component:
Better to just use proxy state.

Example reproduction:

export const Test = () => {
  const snap = useSnapshot(state)
  if(!snap) return null;
  return(
    <></>
  )
}

valtio: 1.7.0
eslint-plugin-valtio: 0.6.0

Following #40.

Valtio's useSnapshot is incompatible with some use cases that are allowed (and recommended) in React.
It would be really nice if we can detect such cases and warn it.

What's not working

const state = proxy({ text: 'hello', count: 0 })
const Component = () => {
  const snap = useSnapshot(state)
  useEffect(() => {
    console.log('something is changed')
  }, [snap])
  return <div>{snap.text}</div>
}

The problem in the example above is when we change the count as ++state.count, the component won't re-render, thus the effect doesn't fire. What's worse is if snap is used in the useEffect callback body, but that's a different story (and already caught by other rules.)

This is not solvable by design and we let people use some workarounds.

Alternative 1

We educate valtio users to use only primitive values for useEffect deps.

The above non-working example would become this:

const state = proxy({ text: 'hello', count: 0 })
const Component = () => {
  const snap = useSnapshot(state)
  const { text, count } = snap // destructure outside useEffect
  useEffect(() => {
    console.log('something is changed')
  }, [text, count])
  return <div>{snap.text}</div>
}

This style doesn't conflict with React linter.

Alternative 2

If we really want to watch the reference change, we shouldn't use snap, but use state instead.

const state = proxy({ text: 'hello', count: 0 })
const Component = () => {
  const snap = useSnapshot(state)
  useEffect(() => subscribe(state, () => {
    console.log('something is changed')
  }), [])
  return <div>{snap.text}</div>
}

It's not exactly the same as the original code intention, but in practice, it would cover most use cases.

Idea

I originally thought this misusage can't be caught by our linter, but if we use TypeScript parser, it should be fairly easy to know if an object is the result of useSnapshot, even for nested objects.

I'm not sure if we can change types only for linter. Or, even without types, eslint has clever enough to do flow analysis?

const { initialized, loading } = useSnapshot(EventsStore);      <-- warning

render (
    {(loading || !initialized) && <Loader text={t("Loading...")} />}
)

valtio/state-snapshot-rule
warning Better to just use proxy state
valtio v0.5.1

I want to update a part of global state in a child component.
Then, eslint-plugin-valtio warn based on valtio/state-snapshot-rule.

How to implement in correct way?

sample:

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

const state = proxy({
  outer: {
    inner: {
      count: 0,
    },
  },
});

export const Parent: FunctionComponent = () => {
  const snapshot = useSnapshot(state);

  return (
    <>
      {snapshot.outer.inner.count}
      {/* Using proxies in the render phase would cause unexpected problems.eslint(valtio/state-snapshot-rule) */}
      <Child prop={state.outer.inner}></Child>
    </>
  );
};

const Child: FunctionComponent<{ prop: { count: number } }> = ({ prop }) => {
  const innerSnapshot = useSnapshot(prop);
  const handleClick = () => prop.count ++;

  return <button onClick={handleClick}>{innerSnapshot.count}</button>;
};

with

valtio: 1.9.0
eslint-plugin-valtio: 0.6.1

valtio/state-snapshot-rule should not show warn in useMemo.

There are situations where useMemo needs to track snapshots.

as descripted here: https://github.com/pmndrs/valtio/wiki/Some-gotchas#using-reactmemo-with-object-props-may-result-in-unexpected-behavior, a rule to check if a prop of Reac.memo component is snapshot would be very helpful