reduxjs / redux-toolkit

The official, opinionated, batteries-included toolset for efficient Redux development
https://redux-toolkit.js.org
MIT License
10.69k stars 1.17k forks source link

Parent/Child component both select Redux state, but only Parent sees changes? #4625

Closed jasonlo87 closed 4 weeks ago

jasonlo87 commented 4 weeks ago

Hello,

I'm probably doing something dumb, but I've been struggling to determine what the problem is here. Essentially I have a Parent component that has a Child component, and when I select the same Redux state in both Parent and Child, only the parent sees updates? When I run and change the state, I see the Parent's "current group outer" log but not the Child's "current group inner" log?

Parent

const SubManagerComponent = () => {
    const currentGroupId = useAppSelector(state => state.subManager.workingState.currentGroup.groupId);
    useEffect(() => {
        console.log(`current group outer is now ${currentGroupId}`)
    }, [currentGroupId])

    return (
        <SubManager/>
    );
}

export default SubManagerComponent;

Child

const SubManager = (props: PropsFromRedux) => {
    const [saveAll] = useSaveAllMutation();
    const [getSubExDetailsForStrategy] = useLazyGetAllSubExDetailsForStrategyQuery();
    const {data: accountsForStrategy} = useGetAllAccountsForStrategyQuery(props.strategy.name);

    const [saveSuccess, setSaveSuccess] = useState(false);
    const [saveFailure, setSaveFailure] = useState(false);
    const [currentGroupName, setCurrentGroupName] = useState("");
    const [cancelConfirmOpen, setCancelConfirmOpen] = useState(false);
    const [saveConfirmOpen, setSaveConfirmOpen] = useState(false);

    const [validationErrorsPopupOpen, setValidationErrorsPopupOpen] = useState(false);
    const [validationErrors, setValidationErrors] = useState<ValidationError[]>([]);

    const currentGroupId = useAppSelector(state => state.subManager.workingState.currentGroup.groupId);

    useEffect(() => {
        console.log(`current group inner is now ${currentGroupId}`)
    }, [currentGroupId])

    // rest of component omitted
}
const connector = connect(mapStateToProps, mapDispatchToProps);
type PropsFromRedux = ConnectedProps<typeof connector>
export default connector(SubManager);

Package Json:

{
  "name": "my proj",
  "version": "1.4.8",
  "type": "module",
  "private": true,
  "dependencies": {
    "@ag-grid-community/client-side-row-model": "^32.1.0",
    "@ag-grid-community/core": "^32.1.0",
    "@ag-grid-community/react": "^32.1.0",
    "@ag-grid-community/styles": "^32.1.0",
    "@ag-grid-enterprise/rich-select": "^32.1.0",
    "@ag-grid-enterprise/row-grouping": "^32.1.0",
    "@emotion/react": "^11.10.4",
    "@emotion/styled": "^11.10.4",
    "@mui/icons-material": "^5.15.19",
    "@mui/lab": "^5.0.0-alpha.170",
    "@mui/material": "^5.15.19",
    "@reduxjs/toolkit": "^2.2.7",
    "better-react-mathjax": "^2.0.3",
    "csstype": "^3.0.8",
    "fast-safe-stringify": "^2.1.1",
    "mathjax": "^3.2.2",
    "react": "^18.3.1",
    "react-dom": "^18.3.1",
    "react-redux": "^9.1.2",
    "react-router": "^6.26.2",
    "react-router-dom": "^6.26.2",
    "typescript": "^5.5.4",
    "uuid": "^10.0.0",
    "web-vitals": "^4.2.3"
  },
  "scripts": {
    "start": "vite dev",
    "build": "mkcert -cert-file localhost.pem -key-file localhost-key.pem localhost.awstrp.net; vite build",
    "build-ci": "vite build -m ci"
  },
  "browserslist": {
    "production": [
      ">0.2%",
      "not dead",
      "not op_mini all"
    ],
    "development": [
      "last 1 chrome version",
      "last 1 firefox version",
      "last 1 safari version"
    ]
  },
  "devDependencies": {
    "@eslint/js": "^9.9.1",
    "@types/node": "^22.5.3",
    "@types/react": "^18.3.5",
    "@types/react-dom": "^18.3.0",
    "@types/react-router": "^5.1.20",
    "@types/react-router-dom": "^5.3.3",
    "@types/uuid": "^10.0.0",
    "@typescript-eslint/eslint-plugin": "^8.1.0",
    "@typescript-eslint/parser": "^8.1.0",
    "@vitejs/plugin-react": "^4.3.1",
    "eslint": "^9.9.1",
    "eslint-plugin-react": "^7.35.2",
    "globals": "^15.9.0",
    "typescript-eslint": "^8.4.0",
    "vite": "^5.4.0"
  }
}
markerikson commented 4 weeks ago

Don't have a specific answer for this one, I'm afraid - they ought to both update. Not sure there's anything I can do without seeing a specific example that reproduces the issue.

(Also, this would really be a React-Redux question, not an RTK question.)

phryneas commented 4 weeks ago

I am confused why there is a connect call as part of all of this though. You shouldn't need that since you're using the hooks.

jasonlo87 commented 4 weeks ago

Thanks for looking so quickly guys!

I've made some progress on this and found the issue since posting, although I would still like you're input on the behavior if you don't mind :)

Basically I had a bug where I was pushing directly into an array that was frozen by Immer in the Child's mapStateToProps (which I didn't include above, so you guys couldn't have known anyway).

So the question is when I was doing that as the code was setup above, I got no error in the console whatsoever abour pushing into the frozen array The child just didn't re-render.

However, if I selected my currentGroupId state in the Parent instead, and passed it to the child as a Prop, I suddenly got a stacktrace about the frozen array push?

jasonlo87 commented 4 weeks ago

I am confused why there is a connect call as part of all of this though. You shouldn't need that since you're using the hooks.

The code's kind of in flux, so there's a mix of both right now. Is that bad practice for some for some reason other than mixing styles?

markerikson commented 4 weeks ago

Yeah, what's happening is that the error is getting thrown in mapState due to attempting to mutate a frozen array, but per https://github.com/reduxjs/react-redux/issues/1942 , that gets swallowed.

(This is yet another reason to migrate away from connect ASAP :) )

All that said, I am very curious how your logic ended up trying to mutate an array in mapState in the first place.

jasonlo87 commented 4 weeks ago

Yeah, what's happening is that the error is getting thrown in mapState due to attempting to mutate a frozen array, but per reduxjs/react-redux#1942 , that gets swallowed.

(This is yet another reason to migrate away from connect ASAP :) )

All that said, I am very curious how your logic ended up trying to mutate an array in mapState in the first place.

It's just bad programming on my part I guess, and now that I know not to do that I can just fix it. Definitely vote for not swallowing errors in the framework if possible tho, this was a 1 minute fix that took like 2 days to find :(

I can migrate to hooks no issue, but the docs I'd read seemed to suggest that while hooks were newer, connect was in some way better because it did some intelligent batching or something, which is why I hadn't banished it yet

markerikson commented 4 weeks ago

why is the error swallowed in one case and not the other?

Because connect is a very complex legacy layer with its own logic, whereas useSelector is basically just React's useSyncExternalStore hook at this point.

Definitely vote for not swallowing errors in the framework if possible tho

Per the linked issue, it's probably fixable in some way, but making changes to connect isn't on our priority list at all at this point, and none of us maintainers have put any thought into it. Open to PRs, but the better answer here is definitely to drop connect and switch to the hooks anyway.

the docs I'd read seemed to suggest that while hooks were newer, connect was in some way better because it did some intelligent batching or something

Definitely not the case, and if anything connect is worse for performance because it has to do some complex cascading of store updates to nested components during the commit phase.

jasonlo87 commented 4 weeks ago

why is the error swallowed in one case and not the other?

Because connect is a very complex legacy layer with its own logic, whereas useSelector is basically just React's useSyncExternalStore hook at this point.

the docs I'd read seemed to suggest that while hooks were newer, connect was in some way better because it did some intelligent batching or something

Definitely not the case, and if anything connect is worse for performance because it has to do some complex cascading of store updates to nested components during the commit phase.

Good Info! Ok I'll switch to selectors, thanks for looking