mobx icon indicating copy to clipboard operation
mobx copied to clipboard

Recommended pattern `MyComp = observer(function MyComp() {...})` leads to subtle bug

Open Venryx opened this issue 1 year ago • 0 comments

Relevant section in the documentation: https://github.com/mobxjs/mobx/blob/b970cbb4dd2e43516d37f3f01c956cab3540d4d3/docs/react-integration.md?plain=1#L370-L378

While the code snippet above works fine for this simple case, it leads to a very confusing and subtle bug, as seen below:

export const TodoItem = observer(function TodoItem(props) {
    const {item} = props;
    return (
        <div>
            <div>{item.text}</div>
            {item.children.map(childItem=>{
                return <TodoItem key={childItem.id} item={childItem}/>;
            })}
        </div>
    );
})

Anyone spot the problem?

The problem is that because the inner TodoItem function is exactly the same name as the outer TodoItem variable, when you reference TodoItem from within the function itself, JavaScript thinks you are referring to that inner function, rather than the outer variable.

Result: The child TodoItem components silently lack any mobx reactivity, due to their not having the observer() wrapper applied to them.

Suggestion: Change the example code to have the inner function named MyComponent_ or MyComponent_impl instead, and add a short note to that section explaining why this was done (so others don't remove that "unnecessary underscore" when writing their actual component, and hit this very subtle bug).


I raise this issue because I had this exact problem, and wasted about half an hour perplexed at why the children components were just inexplicably not responding to any mobx changes, and yet when I made an exact copy of the component and used that for the children, things magically worked again.

Turns out, my "exact copy except I changed the name" was itself how it was fixed, ie. by merely changing the component's name, I somehow fixed the issue. Then the reason that worked finally hit me.

(This issue might seem obvious to people viewing the code above with fresh eyes, but when you're dealing with an actual component with lots of contents, there are other things you're looking at that can distract you from finding the real problem, ie. variable name-shadowing.)

Venryx avatar Feb 22 '24 00:02 Venryx