-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
ApiProvider: correctly register listeners, allow to disable listeners #2277
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit a5551ee:
|
✅ Deploy Preview for redux-starter-kit-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
useEffect( | ||
(): undefined | (() => void) => | ||
props.setupListeners === false | ||
? undefined | ||
: setupListeners(store.dispatch, props.setupListeners), | ||
[props.setupListeners] | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, do we need to rework setupListeners
and initialized
make sure we're cleaning up the subscriptions here if we're putting it in an effect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, that's a good point. Especially with React 18 double-running effects in dev.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setupListeners
returns an unsubscribe
function so this should be fine as it is.
Let's put this in |
5ac9cf1
to
a5551ee
Compare
As mentioned in #2003, might even be worth to put it into a 1.8 patch depending how long we want to wait until 1.9.