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

[v.7.0.0] Component one update behind #1219

Closed
luskin opened this issue Apr 3, 2019 · 19 comments
Closed

[v.7.0.0] Component one update behind #1219

luskin opened this issue Apr 3, 2019 · 19 comments

Comments

@luskin
Copy link

luskin commented Apr 3, 2019

What is the current behavior?

After upgrading to v7.0.0-beta.0 our child components are one update behind the state unless we connect the parent component as well.

Here is a simple test component:

// @flow
import React from 'react'
import { connect } from 'react-redux'

type Props = {
  user: Object,
  organization: Object
}

/**
 *
 * @param {user} Object user from redux state
 * @param {organization} Object organization from redux state
 */
export const TestComponent = ({ user, organization }: Props) => {
  console.log('First Name [Component]: ', user.firstName)
  return <div>{user.firstName}</div>
}

export const MSP = ({ user, organization }: Props) => {
  console.log('First Name [MSP]: ', user.firstName)
  return { user, organization }
}
export default connect(MSP)(TestComponent)

Here is the output using react-redux 6.0.1:

[DEBUG]: Initialize user module
First Name [MSP]:  Greg
First Name [Component]:  Greg
--- HERE IS WHERE THE STATE WAS UPDATED ---
First Name [MSP]:  Gregg
First Name [Component]:  Gregg

here is the output using react-redux 7.0.0-beta.0

[DEBUG]: Initialize user module
First Name [MSP]:  Greg
First Name [Component]:  Greg
--- HERE I UPDATED STATE FROM Greg to Gregg ---
First Name [MSP]:  Gregg
--- NOTICE THAT COMPONENT WAS NOT UPDATED ---
--- I THEN PROCEED TO UPDATE THE STATE TO Greggg ---
First Name [MSP]:  Greggg
First Name [Component]:  Gregg

As you can see mapStateToProps requires the state to be modified a second time before updating the component to the previous state. It seems as though possibly the comparison check is not firing off?

Things I have tried:

  • Spreading the objects into a new object (new reference, comparison should be false and thus a rerender), this did not work.
  • Updating the functional component to a PureComponent and then Component, this did not work.
  • Removed all hooks from parent component, this did not work
  • Connecting parent component and mapStateToProps-ing the user object, THIS DID WORK

What is the expected behavior?
Redux should do an equality check and notice that the state is different and update the component props accordingly.

React 16.8.5
React-Redux: 7.0.0-beta.0

@markerikson
Copy link
Contributor

Can you please provide a sample project that reproduces the issue, either as a CodeSandbox or a cloneable repo?

@luskin
Copy link
Author

luskin commented Apr 3, 2019

@markerikson I just spend about 30 mins trying to replicate it in a simple project, unfortunately the problem is not present in a simple implementation. I cannot share the project that this issue is occurring in as it is a core product of my company.

One thing I noticed when trying to replicate this in a side project is that when updating the state from UI (i.e. a button) it seems to work fine in v7.0.0-beta.0 however when it's updated from within a saga it does not force the re-render.

I wish I could get this replicated in a project I am allowed to share. We'll see if anyone else pops in with this issue and can better provide a replication source.

For the time being we will drop back to 6.0.1 and continually try 7 as it becomes more mature.

@markerikson
Copy link
Contributor

I would really appreciate it if you could try to replicate this further. At a minimum, could you give some examples of where and how you are triggering state updates in relation to that component snippet?

This is exactly the kind of feedback I'm looking for to determine whether v7 is ready for final release. If there are issues, I want to resolve them now, but I need more info to be able to dig in myself.

@josepot
Copy link
Contributor

josepot commented Apr 3, 2019

Hi @luskin I'm trying to help at reproducing this, because I think that I experienced something similar yesterday.

One question, though, when you say that:

child components are one update behind the state unless we connect the parent component as well

What do you mean exactly?

Do you mean that this only happens with a connected component that doesn't have any other connected components among its ancestors? I mean, are you saying that this problem doesn't happen with connected components that are "nested" with other connected components? Could you please elaborate more on this?

Thanks!

@luskin
Copy link
Author

luskin commented Apr 3, 2019

One thing to note that I did not mention previously is that we are using redux-dynamic-modules from Microsoft. It is possible that there is a mismatch between that package and redux 7. However for sake of the issue I will continue...

After a user is authenticated (firebase auth) in our <RedirectHandler /> we return a <CustomerDashboard /> component. On customer dashboard load we turn on firebase database listeners for the user object. These listeners are built with a saga channel/event emitter. On any change we fire off a child_update, child_added, child_removed dispatch action and reduce the new user object.

<CustomerDashboard /> connects to redux for user updates and here in-lies the issue, this is where the user object is always one update behind the stored state. When inspecting the redux state the user gets updated properly and the mapStateToProps get's called accordingly but it does not trigger a re-render of the <CustomerDashboard /> component and thus does not trickle changes down.

IF we connect our <RedirectHandler /> to redux and return the user object to props then everything works as expected, but this is a hack.

I'll share these two components and they do not expose anything sensitive:

CustomerDashboard:

// @flow
import React from 'react'
import { connect } from 'react-redux'
import Dashboard from 'views/dashboard'
import SideNav from 'views/customer/sideNav'
import CustomerRoutes from 'router/customer'
import { Wrapper, PageContent } from './style'

// Dynamic Modules
import { DynamicModuleLoader } from 'redux-dynamic-modules'
import shipmentsModule from 'store/modules/shipments'
import userModule from 'store/modules/user'
import organizationModule from 'store/modules/organization'

// Contexts
import UserContext from 'contexts/user/index'
import OrganizationContext from 'contexts/organization/index'

type Props = {
  user: Object,
  organization: Object
}

/**
 *
 * @param {user} Object passes in the original user object obtained from the identification process
 * @param {organization} Object passes in the original organization object ob tained from the identification process
 */
export const CustomerDashboard = ({ user, organization }: Props) => {
  return (
    <DynamicModuleLoader
      modules={[userModule(user), organizationModule(organization), shipmentsModule()]}
    >
      <UserContext.Provider value={user}>
        <OrganizationContext.Provider value={organization}>
          <Dashboard>
            <Wrapper>
              <SideNav />
              <PageContent>
                <CustomerRoutes />
              </PageContent>
            </Wrapper>
          </Dashboard>
        </OrganizationContext.Provider>
      </UserContext.Provider>
    </DynamicModuleLoader>
  )
}
export const MSP = ({ user, organization }: Props) => ({ user, organization })
export default connect(MSP)(CustomerDashboard)

RedirectHandler:

// @flow
import React, { useContext } from 'react'
import AuthContext from 'contexts/auth'
import useIdentificationProcess from './useIdentificationProcess'
import isLoadingApplication from './helpers/isLoadingApplication'
import isShipperOrganization from './helpers/isShipperOrganization'

// Views
import Authentication from 'views/authentication'
import ApplicationLoadingView from 'views/applicationLoading'
import CustomerDashboard from 'views/customer/dashboard'

export const RedirectHandler = () => {
  const auth = useContext(AuthContext)
  const { user, organization } = useIdentificationProcess()
  const isLoading = isLoadingApplication({ auth, user, organization })
  const isShipper = isShipperOrganization({ organization })

  // App Loading View
  if (isLoading) {
    return <ApplicationLoadingView />
  }

  // Unathenticated Router
  if (!isLoading && !auth.uid) {
    return <Authentication />
  }

  // Authenticated Router
  if (isShipper) {
    const passProps = { user, organization }
    return <CustomerDashboard {...passProps} />
  }

  // TODO: Log error and placeholder page for user
  return <div>Unknown Redirect</div>
}
export default RedirectHandler

App:

// @flow
import React from 'react'
import useAuth from 'hooks/useAuth'
import RouteChangeListener from 'components/routeChangeListener'
import GlobalStyle from 'config/globalStyles'
import Segment from 'components/segment'
import { configureStore } from 'store'
import history from 'router/history'
import theme_default from 'config/theme/default'

// Components
import RedirectHandler from 'components/redirectHandler'
import Modal from 'views/modal'

// Providers
import { Provider } from 'react-redux'
import { Router } from 'react-router-dom'
import { ThemeProvider } from 'styled-components'
import AuthContext from 'contexts/auth'

// Dynamic Modules
import { DynamicModuleLoader } from 'redux-dynamic-modules'
import modalModule from 'store/modules/modal'
import navigationModule from 'store/modules/navigation'

const store = configureStore()

export const App = () => {
  const authState = useAuth()
  return (
    <Provider store={store}>
      <Router history={history}>
        <ThemeProvider theme={theme_default}>
          <DynamicModuleLoader modules={[modalModule(), navigationModule()]}>
            <AuthContext.Provider value={authState}>
              <Segment />
              <RouteChangeListener />
              <GlobalStyle />
              <RedirectHandler />
              <Modal />
            </AuthContext.Provider>
          </DynamicModuleLoader>
        </ThemeProvider>
      </Router>
    </Provider>
  )
}
export default App

@navneet-g
Copy link

@luskin can you provide version details of all packages you are using? You can remove any private packages.

@luskin
Copy link
Author

luskin commented Apr 3, 2019

@josepot To be clearer what I'm saying is that this problem occurs when the component has NO connected parents. Once we connect the parent component the updates are synced back up.

In my example code above the problem is solved by adding:

export const MSP = state => ({ user: state.user })
export default connect(MSP)(RedirectHandler)

to RedirectHandler

@navneet-g Sure thing, it's pretty simple so far since we are still in the early phases of the rebuild:

    "firebase": "^5.9.1",
    "flow-bin": "^0.95.1",
    "formik": "^1.5.1",
    "moment": "^2.24.0",
    "moment-timezone": "^0.5.23",
    "react": "^16.8.6",
    "react-detect-offline": "^2.1.2",
    "react-dom": "^16.8.5",
    "react-helmet": "^6.0.0-beta",
    "react-input-mask": "^2.0.4",
    "react-pose": "^4.0.8",
    "react-redux": "^6.0.1",
    "react-router-dom": "^5.0.0",
    "react-scripts": "^2.1.8",
    "redux": "^4.0.1",
    "redux-dynamic-modules": "^3.5.0",
    "redux-dynamic-modules-saga": "^3.5.0",
    "redux-saga": "^1.0.2",
    "styled-components": "^4.2.0",
    "use-onclickoutside": "^0.3.1",
    "yup": "^0.27.0"

^ we dropped react-redux back down to 6.0.1 to continue development

@josepot
Copy link
Contributor

josepot commented Apr 3, 2019

Thanks a lot @luskin for providing all those details. One last question: could you please share the content of your custom hook: useIdentificationProcess? If you can't, could you at least explain what is it that it's doing?

The reason why I'm asking is because it seems to me that this hook and the connect HOC are "competing" for passing down the same properties to CustomerDashboard. I'm worried about a possible "race condition", where one could be overriding the other. I hope that makes sense 🙂

@luskin
Copy link
Author

luskin commented Apr 3, 2019

@josepot I thought this too and refactored a version that did not pass the props down in this way, however it did not make any difference and so I reverted back to the cleaner (imo) implementation.

The issue exists whether or not useIdentificationProcess is in the component.

@navneet-g
Copy link

Good news ... I have a repro ... I will upload it to code sandbox or create a repo to share soon. I have not debugged it yet to see what might be happening.

@navneet-g
Copy link

navneet-g commented Apr 4, 2019

Here is the repo with repro, the readme has repro steps
https://github.com/navneet-g/react-redux-beta-07-repro
And here is the codesandbox, the repro steps are same.
https://codesandbox.io/s/github/navneet-g/react-redux-beta-07-repro

@navneet-g
Copy link

navneet-g commented Apr 4, 2019

Just to be sure ... I verified the same code base, with react-redux v6 and the issue does not repro, it tells me that the bug is related to react-redux beta or some where in integration :).

@navneet-g
Copy link

navneet-g commented Apr 4, 2019

So far ... connectAdvanced dispatches following action, but the render method is not called on the component until next action is dispatched. Does this mean this is a bug in useReducer ?

image

@josepot
Copy link
Contributor

josepot commented Apr 4, 2019

Does this mean this is a bug in useReducer

I don't think so, because the render is being triggered. I think that the problem is that renderedChild is not being re-evaluated, because none of the dependencies of its useMemo hook have changed since the last render (in this case actualChildProps does not change). I think that this is the issue.

josepot added a commit to josepot/react-redux-lean that referenced this issue Apr 4, 2019
@josepot
Copy link
Contributor

josepot commented Apr 4, 2019

I can confirm that #1220 fixes the issue locally 🙂 I'm now creating a test-case for the PR.

@navneet-g
Copy link

Awesome .. from report to fix in 6 hours :)

@josepot
Copy link
Contributor

josepot commented Apr 4, 2019

The power of teamwork! 😊

@markerikson
Copy link
Contributor

Just merged the fix and published 7.0.0-beta.1. Can folks try it out and see if it works, especially @luskin ?

@luskin
Copy link
Author

luskin commented Apr 4, 2019

Confirmed fix on my end, we will continue implementation with v7!

@luskin luskin closed this as completed Apr 4, 2019
markerikson pushed a commit that referenced this issue Apr 9, 2019
* Update React to latest

* Update React peer dependency to 16.8.x

* Initial re-implementation of `connectAdvanced` using hooks

Matches changes from v7.0.0-alpha.1

* Update tests to match v7-alpha.1 behavior

Added rtl.act() calls around dispatches and other component updates
Added clarification on expected mapState calls in some places
Disabled some no-longer-relevant tests per implementation
Made tests run against React 16.8 by default

* adding a react hooks test that fails with v7 alpha

* wrapping store.dispatch with rlt.act, fixes component renders

* reducing hooks test to 2 components

* Fix case where wrapper props changed before store update render

* Mark ReactDOM as global in UMD builds

Matches state as of v7.0.0-alpha.2

* Fix perf problems with out-of-bounds array access

Matches state as of v7.0.0-alpha.3

* Add modules to handle importing batchedUpdates

* Use appropriate batched update API for subscriptions

* Inject unstable_batchedUpdates in default entry point

* Provide an alternate entry point for alternate renderers

Matches state as of v7.0.0-alpha.4

* Remove batch arg from createListenerCollection (#1205)

This prevents a bug with Terser (webpack's default minifier) where the
returned batch function isn't defined due to function inlining.

Matches state as of v7.0.0-alpha.5

* Remove older React versions from Travis

* Add comments to connectAdvanced. Many much comments!

* Re-add test for a custom store as a prop

* Fix null pointer exception when store is given as a prop

We were trying to read contextValue.subscription, even if that
value was null.  Reworked logic to handle cases where the store
came in as a prop.

* Ensure wrapper props are passed correctly when forwarding refs

* Add a test to verify subscription passthrough with store-as-prop

* add non-batched tests (#1208)

* Force SSR tests to mimic a Node environment

* Restructure connect tests to group by category for easier reading

Yeah, this kills the blame history. Sorry. But it's a lot easier
to figure out what the tests are used for now.

* Clean up dead code in Provider tests

* Add tests to verify errors are thrown for bad mapState functions

* Fix edge cases around saved wrapper props and error handling

Changed to useLayoutEffect to ensure that the lastWrapperProps ref
is written to synchronously when a render is committed. However,
because React warns about this in SSR, added a check to fall back
to plain useEffect under Node so we don't print warnings.

Also added logic to ensure that if an error is thrown during a
mapState function, but the component is unmounted before it can
render, that the error will still be thrown.  This shouldn't happen
given our top-down subscriptions, but this will help surface the
problem in our tests if we ever break the top-down behavior.

* Formatting

* Add a test to verify no errors are printed in SSR usage

* Ignore .idea/

* 7.0.0-beta.0

* Updated outdated SSR-test (dispatch in ancestors) (#1213)

* Added test for injecting dynamic reducers on client and server (#1211)

* Remove WebStorm gitignore

This goes in a global gitignore file, not a project.

* [FIX]: #1219 Save references before update (#1220)

* Re-ignore .idea/

* 7.0.0-beta.1

* Update the codecov config to be more forgiving.

* add test to verify that mapStateToProps is always called with latest store state (#1215)
albertodev7 pushed a commit to albertodev7/react-redux that referenced this issue Dec 8, 2022
* Update React to latest

* Update React peer dependency to 16.8.x

* Initial re-implementation of `connectAdvanced` using hooks

Matches changes from v7.0.0-alpha.1

* Update tests to match v7-alpha.1 behavior

Added rtl.act() calls around dispatches and other component updates
Added clarification on expected mapState calls in some places
Disabled some no-longer-relevant tests per implementation
Made tests run against React 16.8 by default

* adding a react hooks test that fails with v7 alpha

* wrapping store.dispatch with rlt.act, fixes component renders

* reducing hooks test to 2 components

* Fix case where wrapper props changed before store update render

* Mark ReactDOM as global in UMD builds

Matches state as of v7.0.0-alpha.2

* Fix perf problems with out-of-bounds array access

Matches state as of v7.0.0-alpha.3

* Add modules to handle importing batchedUpdates

* Use appropriate batched update API for subscriptions

* Inject unstable_batchedUpdates in default entry point

* Provide an alternate entry point for alternate renderers

Matches state as of v7.0.0-alpha.4

* Remove batch arg from createListenerCollection (reduxjs#1205)

This prevents a bug with Terser (webpack's default minifier) where the
returned batch function isn't defined due to function inlining.

Matches state as of v7.0.0-alpha.5

* Remove older React versions from Travis

* Add comments to connectAdvanced. Many much comments!

* Re-add test for a custom store as a prop

* Fix null pointer exception when store is given as a prop

We were trying to read contextValue.subscription, even if that
value was null.  Reworked logic to handle cases where the store
came in as a prop.

* Ensure wrapper props are passed correctly when forwarding refs

* Add a test to verify subscription passthrough with store-as-prop

* add non-batched tests (reduxjs#1208)

* Force SSR tests to mimic a Node environment

* Restructure connect tests to group by category for easier reading

Yeah, this kills the blame history. Sorry. But it's a lot easier
to figure out what the tests are used for now.

* Clean up dead code in Provider tests

* Add tests to verify errors are thrown for bad mapState functions

* Fix edge cases around saved wrapper props and error handling

Changed to useLayoutEffect to ensure that the lastWrapperProps ref
is written to synchronously when a render is committed. However,
because React warns about this in SSR, added a check to fall back
to plain useEffect under Node so we don't print warnings.

Also added logic to ensure that if an error is thrown during a
mapState function, but the component is unmounted before it can
render, that the error will still be thrown.  This shouldn't happen
given our top-down subscriptions, but this will help surface the
problem in our tests if we ever break the top-down behavior.

* Formatting

* Add a test to verify no errors are printed in SSR usage

* Ignore .idea/

* 7.0.0-beta.0

* Updated outdated SSR-test (dispatch in ancestors) (reduxjs#1213)

* Added test for injecting dynamic reducers on client and server (reduxjs#1211)

* Remove WebStorm gitignore

This goes in a global gitignore file, not a project.

* [FIX]: reduxjs#1219 Save references before update (reduxjs#1220)

* Re-ignore .idea/

* 7.0.0-beta.1

* Update the codecov config to be more forgiving.

* add test to verify that mapStateToProps is always called with latest store state (reduxjs#1215)
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