pmndrs / valtio

šŸ§™ Valtio makes proxy-state simple for React and Vanilla
https://valtio.dev
MIT License
9.16k stars 257 forks source link

TypeError: 'set' on proxy: trap returned falsish for property '0' #205

Closed Nunatic02 closed 3 years ago

Nunatic02 commented 3 years ago

I'm working with Three.js. I've set up a store in a file out of all react components as

const appState = proxy({
  scene: null!,
})
export default appState

I then init the scene in the initialization code as

const scene = new THREE.Scene();
appState.scene = scene;

In a React component, I get a snapshot of the scene as

const { scene } = useSnapshot(appState);

And try to add a new group to the scene when I click on a menu item as

 <MenuItem
    onClick={() => {
      scene.add(new THREE.Group());
    }}
>
    Add a New Group
</MenuItem>

This will cause an error shown as below:

CleanShot 2021-08-02 at 18 38 01@2x

where the scene.add() function has source code as below:


    add( object ) {

        if ( arguments.length > 1 ) {

            for ( let i = 0; i < arguments.length; i ++ ) {

                this.add( arguments[ i ] );

            }

            return this;

        }

        if ( object === this ) {

            console.error( 'THREE.Object3D.add: object can\'t be added as a child of itself.', object );
            return this;

        }

        if ( object && object.isObject3D ) {

            if ( object.parent !== null ) {

                object.parent.remove( object );

            }

            object.parent = this;
            this.children.push( object );

            object.dispatchEvent( _addedEvent );

        } else {

            console.error( 'THREE.Object3D.add: object not an instance of THREE.Object3D.', object );

        }

        return this;

    }

I know that I can use ref(scene) to get rid of the error, but I do want to track any changes that happen on the scene object.

Nunatic02 commented 3 years ago

Ah! I realized that I should modify the state instead of the snapshot. šŸ„² Sorry about that!

dai-shi commented 3 years ago

Well, it's actually good you reported. you voted up.

https://github.com/pmndrs/valtio/issues/204#issuecomment-890858854

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

deadcoder0904 commented 3 years ago

@dai-shi I get a similar error:

TypeError: 'set' on proxy: trap returned falsish for property 'title' at Proxy.setTitle (ink-cli\scripts\new-post\dist\state.js:21:20) at onSubmit (ink-cli\scripts\new-post\dist\ui.js:22:22) at handleSubmit (ink-cli\scripts\new-post\dist\components\Input.js:18:9)

I'm using it with ink

state.ts

import { proxy } from 'valtio'

class State {
    title = ''
    description = ''

    setTitle(title: string) {
        this.title = title
    }

    setDescription(description: string) {
        this.description = description
    }
}

export const state = proxy(new State())

ui.tsx

import React from 'react'
import { useSnapshot } from 'valtio'
import { Box } from 'ink'
import Gradient from 'ink-gradient'
import BigText from 'ink-big-text'

import { state } from './state'
import { Input } from './components/Input'

interface IApp {}

const App = ({}: IApp) => {
    const snap = useSnapshot(state)

    return (
        <Box flexDirection="column">
            <Gradient name="fruit">
                <BigText text="Compose New Post" font="tiny" />
            </Gradient>
            <Input
                label="Title"
                onChange={(title: string) => {
                    return title
                }}
                onSubmit={(title) => {
                    console.log('title', title)
                    snap.setTitle(title)
                }}
            />
            <Input
                label="Description"
                onChange={(description: string) => {
                    return description
                }}
                onSubmit={(description) => {
                    console.log('description', description)
                    snap.setDescription(description)
                }}
            />
        </Box>
    )
}

module.exports = App
export default App

Haven't found a solution for my problem though :(

dai-shi commented 3 years ago

@deadcoder0904

snap.setTitle(title)

This is trying to set the title of the snapshot, which is not modifiable. Because you want to modify the state, the following is correct.

state.setTitle(title)

We have https://github.com/pmndrs/eslint-plugin-valtio to detect this. Please try it and if doesn't work, open an issue there.

deadcoder0904 commented 3 years ago

@dai-shi I tried it & it did give a warning in VSCode saying:

Better to just use proxy state.eslint(valtio/state-snapshot-rule)
const snap: {
    title: string;
    description: string;
    setTitle: (title: string) => void;
    setDescription: (description: string) => void;
}

So then I tried using

snap.title = title

instead of

setTitle(title)

And it still gives the same warning. I want to know how to fix it?

All I want to do is store the state in Valtio of a simple string.

dai-shi commented 3 years ago

@deadcoder0904 Oh, that message is not very friendly šŸ˜“ .

You need to do:

state.title = title

or

state.setTitle(title)
deadcoder0904 commented 3 years ago

@dai-shi But why? Why wouldn't snap work?

const snap = useSnapshot(state)
// snap.title = title
// or snap.setTitle(title)

I can't understand why snap is needed then as it's never used?

The docs are correctly using snap & state but those 2 variables are so close alphabetically that I didn't even check if it was snap or state, just read the count part šŸ˜‚

dai-shi commented 3 years ago

snap is a snapshot (of a state at a certain time), which is an immutable object in valtio. It's frozen.

So, you can't modify snap.

snap.title = 'foo' // doesn't work because snap is frozen.
geocine commented 3 years ago

I am also kinda confused by this, how should this be correctly done @dai-shi ?

store.ts

export interface VStore {
  categories: Category[];
  loadCategories: () => void;
}

export const vstore = proxy<VStore>({
  categories: [],  
  loadCategories // some method
});

Presentation UI

const HomePage = () => {
  const data = useSnapshot(vstore);

  useEffect(() => {
    const loadData = async () => {
      await data.loadCategories();
    };
    loadData();
  }, []);

  return (
    <>
      {/* Some more components here */}
      <CategorySlider categories={data.categories} />
    </>
  );
};

export default HomePage;

CategorySlider Component

const CategorySlider = ({
  categories = []
}: CategorySliderProps) => {
  const [categoryList, setCategoryList] = useState(categories);

  useEffect(() => {
    setCategoryList(categories);
  }, [categories]);

  const selectCategory = (name: string) => () => {
    const newCategoryList = categoryList.map((category: Category) => {
      category.selected = false; // I get an error here
      if (category.name === name) {
        category.selected = true;
      }
      return category;
    });
    setCategoryList(newCategoryList);
  };

  return (
    <Slides>
      {categoryList.map((category: Category, idx: number) => {
        return (
          <Slide key={idx}>
            <CategoryCard
              selected={category.selected}
              onClick={selectCategory(category.name)}
            />
          </Slide>
        );
      })}
    </Slides>
  );
};

export default CategorySlider;

I still cannot modify the data even I passed it down as a prop and used useState?

dai-shi commented 3 years ago

@geocine There are several ways to fix this. One way is to use useSnapshot inside CategorySlider, which does more fine-grained render optimization. The other is to pass the callback for the mutation.

const HomePage = () => {
  const data = useSnapshot(vstore)
  // ...
  return (
    <>
      {/* Some more components here */}
      <CategorySlider categories={data.categories} selectCategory={(idx, value) => { vstore.categories[idx].selected = value } />
    </>
  )
}

const CategorySlider = ({
  categories,
  selectCategory,
}: {
  categories: Category[]
  selectCateogry: (idx: number, value: boolean) => void
}) => {
  // ...
}

Also, this šŸ‘‡ works fine, because useEffect is outside render.

-       await data.loadCategories();
+       await vstore.loadCategories();
geocine commented 3 years ago

Thanks @dai-shi your explanation is very clear to me. Your work made me realize I've been doing state mutations wrong all along. Now this makes me more aware because it actually prevents me from doing this kind of stuff. Really great work!

This is very useful , as doing this , exhaustive deps on useEffect would no longer complain about not adding data as a dependency

-       await data.loadCategories();
+       await vstore.loadCategories();