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

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

Closed
jasonlo87 opened this issue Sep 15, 2024 · 8 comments
Closed

Comments

@jasonlo87
Copy link

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

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.)

@markerikson markerikson closed this as not planned Won't fix, can't repro, duplicate, stale Sep 15, 2024
@phryneas
Copy link
Member

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

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

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

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.

@jasonlo87
Copy link
Author

jasonlo87 commented Sep 15, 2024

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

markerikson commented Sep 15, 2024

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

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

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

3 participants