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

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

Closed
Nunatic02 opened this issue Aug 2, 2021 · 11 comments
Closed

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

Nunatic02 opened this issue Aug 2, 2021 · 11 comments

Comments

@Nunatic02
Copy link

Nunatic02 commented Aug 2, 2021

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
Copy link
Author

Ah! I realized that I should modify the state instead of the snapshot. 🥲 Sorry about that!

@dai-shi
Copy link
Member

dai-shi commented Aug 2, 2021

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

#204 (comment)

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

@deadcoder0904
Copy link

@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
Copy link
Member

dai-shi commented Aug 3, 2021

@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
Copy link

@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
Copy link
Member

dai-shi commented Aug 3, 2021

@deadcoder0904
Oh, that message is not very friendly 😓 .

You need to do:

state.title = title

or

state.setTitle(title)

@deadcoder0904
Copy link

deadcoder0904 commented Aug 3, 2021

@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
Copy link
Member

dai-shi commented Aug 3, 2021

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
Copy link

geocine commented Aug 21, 2021

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
Copy link
Member

dai-shi commented Aug 21, 2021

@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
Copy link

geocine commented Aug 22, 2021

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();

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

4 participants