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.

Sign up for GitHub

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

fix CM tearing #1455

Closed
wants to merge 9 commits into from
Closed

fix CM tearing #1455

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b986030
fix cm use selector
salvoravida Nov 8, 2019
9c0520f
fix cm on connect
salvoravida Nov 9, 2019
3d52f3a
update test for child-parent double render
salvoravida Nov 9, 2019
647195d
save a ref when selector does not trigger an update
salvoravida Nov 9, 2019
d248d0a
add CM tests
salvoravida Nov 10, 2019
eb6f8cf
fix possible falsy on reduxState
salvoravida Nov 11, 2019
6647836
update cm tests @dai-shi
salvoravida Nov 11, 2019
5ed652c
removed useRef - fixed new test on CM
salvoravida Nov 11, 2019
16a6253
removed useRef - fixed new test on CM
salvoravida Nov 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 19 additions & 17 deletions src/components/connectAdvanced.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
import hoistStatics from 'hoist-non-react-statics'
import invariant from 'invariant'
import React, { useContext, useMemo, useRef, useReducer } from 'react'
import React, { useContext, useMemo, useRef, useState } from 'react'
import { isValidElementType, isContextConsumer } from 'react-is'
import Subscription from '../utils/Subscription'
import { useIsomorphicLayoutEffect } from '../utils/useIsomorphicLayoutEffect'

import { ReactReduxContext } from './Context'

// Define some constant arrays just to avoid re-creating these
const EMPTY_ARRAY = []
const NO_SUBSCRIPTION_ARRAY = [null, null]

const stringifyComponent = Comp => {
Expand All @@ -19,13 +18,6 @@ const stringifyComponent = Comp => {
}
}

function storeStateUpdatesReducer(state, action) {
const [, updateCount] = state
return [action.payload, updateCount + 1]
}

const initStateUpdates = () => [null, 0]

export default function connectAdvanced(
/*
selectorFactory is a func that is responsible for returning the selector function used to
Expand Down Expand Up @@ -232,9 +224,9 @@ export default function connectAdvanced(
// We need to force this wrapper component to re-render whenever a Redux store update
// causes a change to the calculated child component props (or we caught an error in mapState)
const [
[previousStateUpdateResult],
previousStateUpdateResult,
forceComponentUpdateDispatch
] = useReducer(storeStateUpdatesReducer, EMPTY_ARRAY, initStateUpdates)
] = useState({ reduxStore: store.getState() })

// Propagate any mapState/mapDispatch errors upwards
if (previousStateUpdateResult && previousStateUpdateResult.error) {
Expand All @@ -247,6 +239,8 @@ export default function connectAdvanced(
const childPropsFromStoreUpdate = useRef()
const renderIsScheduled = useRef(false)

const latestReduxStore = useRef()

const actualChildProps = usePureOnlyMemo(() => {
// Tricky logic here:
// - This render may have been triggered by a Redux store update that produced new child props
Expand All @@ -265,8 +259,15 @@ export default function connectAdvanced(
// This will likely cause Bad Things (TM) to happen in Concurrent Mode.
// Note that we do this because on renders _not_ caused by store updates, we need the latest store state
// to determine what the child props should be.
return childPropsSelector(store.getState(), wrapperProps)
}, [store, previousStateUpdateResult, wrapperProps])
return childPropsSelector(
latestReduxStore.current || previousStateUpdateResult.reduxStore,
wrapperProps
)
}, [
latestReduxStore.current,
previousStateUpdateResult.reduxStore,
wrapperProps
])

// We need this to execute synchronously every time we re-render. However, React warns
// about useLayoutEffect in SSR, so we try to detect environment and fall back to
Expand Down Expand Up @@ -322,6 +323,8 @@ export default function connectAdvanced(

// If the child props haven't changed, nothing to do here - cascade the subscription update
if (newChildProps === lastChildProps.current) {
latestReduxStore.current = latestStoreState
timdorr marked this conversation as resolved.
Show resolved Hide resolved

if (!renderIsScheduled.current) {
notifyNestedSubs()
}
Expand All @@ -335,11 +338,10 @@ export default function connectAdvanced(
renderIsScheduled.current = true

// If the child props _did_ change (or we caught an error), this wrapper component needs to re-render
latestReduxStore.current = undefined
forceComponentUpdateDispatch({
type: 'STORE_UPDATED',
payload: {
error
}
error,
reduxStore: latestStoreState
})
}
}
Expand Down
11 changes: 6 additions & 5 deletions src/hooks/useSelector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { useReducer, useRef, useMemo, useContext } from 'react'
import { useRef, useMemo, useContext, useState } from 'react'
import invariant from 'invariant'
import { useReduxContext as useDefaultReduxContext } from './useReduxContext'
import Subscription from '../utils/Subscription'
Expand All @@ -13,7 +13,7 @@ function useSelectorWithStoreAndSubscription(
store,
contextSub
) {
const [, forceRender] = useReducer(s => s + 1, 0)
const [reduxState, forceRender] = useState(store.getState())

const subscription = useMemo(() => new Subscription(store, contextSub), [
store,
Expand All @@ -31,7 +31,7 @@ function useSelectorWithStoreAndSubscription(
selector !== latestSelector.current ||
latestSubscriptionCallbackError.current
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be CM friendly, could we do something like:

try {
 ...
} catch(e) {
  if (typeof e === 'object' && e !== null && typeof e.then === 'function') throw e
  let errorMessage = ...

So we could do this, for example:

  const status = useSelector(s => {
    if (s.status === STATUS.PENDING) throw promise;
    return s.status;
  })
}

In React docs, it's using something like:
const value = resource.read()
where read() either returns the value or throws so it can bail out before the selector is set. Not sure if here is the best place to do it, but yeah, with CM not everything "thrown" is an error.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry i think that thow promise to comunicate with Suspense does NOT matter with useSelector, that has to select a state without tearing on render. that's another problem.

) {
selectedState = selector(store.getState())
selectedState = selector(reduxState)
} else {
selectedState = latestSelectedState.current
}
Expand All @@ -53,8 +53,9 @@ function useSelectorWithStoreAndSubscription(

useIsomorphicLayoutEffect(() => {
function checkForUpdates() {
const newReduxState = store.getState()
try {
const newSelectedState = latestSelector.current(store.getState())
const newSelectedState = latestSelector.current(newReduxState)

if (equalityFn(newSelectedState, latestSelectedState.current)) {
return
Expand All @@ -69,7 +70,7 @@ function useSelectorWithStoreAndSubscription(
latestSubscriptionCallbackError.current = err
}

forceRender({})
forceRender(newReduxState)
timdorr marked this conversation as resolved.
Show resolved Hide resolved
}

subscription.onStateChange = checkForUpdates
Expand Down
11 changes: 7 additions & 4 deletions test/components/Provider.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,13 +246,13 @@ describe('React', () => {
store.dispatch({ type: 'APPEND', body: 'c' })
})

expect(childMapStateInvokes).toBe(2)
expect(childCalls).toEqual([['a', 'a'], ['ac', 'ac']])
expect(childMapStateInvokes).toBe(3)
expect(childCalls).toEqual([['a', 'a'], ['a', 'ac'], ['ac', 'ac']])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this changes the whole point of the test. this test ensures that mapState never sees inconsistent state. with this change you are asserting the exact opposite

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we NO more read the last state in the render phase, the "parent-child-issue" is resolved with 2 render, as every change of the store make 2 event-callback one on the parent and one on the child.
Before the issue was resolved with the "hack" of reading always the fresh Store state, even if we are on the "preview" scheduled refresh came out from "checkForUpdate" callback.

That hack while resolve the "parent-issue" with 1 render less, broke the changes normal queue on the the componente, that produce tearing on CM where every component could rendered in a random way, jumping the "changhes queue"

please try the demo of @dai-shi with my version and with old one. you will see the differences on a fast store changes, with slow render.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are developing your own fork, it's fine and I think it's one possible approach.
But, if you would like to support all existing RR apps (which is necessary for the official RR), you want to keep the backward compatibility. As I said, it should be technically possible. Seems like you are used to the code base, I think you can do it. I'm not 100% sure though.

Copy link
Author

@salvoravida salvoravida Nov 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dai-shi i'm not dev my own fork, otherwise i havent opened a pr. that said i think that was a mistake reading fresh store state on render, to have 1 less render on that particular use case. Moreover it is of course back compatible with 1 render more (for that usecase)
Anyway i will try to explain better with a flow chart as soon as i can. That's all.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Exactly point is: when a componet render it MUST read the storeState that was from HIS last storeChangeCallback!. Otherwise reading in render store.getState() could be newer than HIS callback queque. THAT'S THE POINT. Think more about that. test the demo.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify what you mean by "more renders"? It sounds like you're saying we will cause additional render cycles to occur, which is going to be bad for performance.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@markerikson I believe this is not due to an additional render. let me try to outline the order of events here:

  1. the store updates
  2. the parent sees the change first (due to tiered subscription) and renders
  3. since the parent rendered and changed the props, the child renders as well. with this change, during the render the child will see the last state it knows about, which is the previous store state (since due to tiered subscription the child didn't see the update yet). funnily enough this is actually the inverse of the stale props issue, since now we actually see newer parent props with an old state instead of old props with a new state
  4. once the render of the parent finishes the child finally sees the state update and re-renders itself with consistent props and state

it feels to me that tiered subscription and CM might just be incompatible

@salvoravida as @dai-shi pointed out, this discussion is not about the correctness of your change, but about preserving the existing behavior for backwards compatibility. there are likely tons of apps out there that have mapStateToProps written in a way that depends on the props and state being consistent. that's why it is important we think of a way to keep the existing behavior. in the useSelector hook we took the approach of simply ignoring these situations and dealing with errors by forcing a re-render. that was a deliberate decision we could make because it was a new API. connect is sadly different


// setState calls DOM handlers are batched
const button = tester.getByText('change')
rtl.fireEvent.click(button)
expect(childMapStateInvokes).toBe(3)
expect(childMapStateInvokes).toBe(5)

// Provider uses unstable_batchedUpdates() under the hood
rtl.act(() => {
Expand All @@ -261,11 +261,14 @@ describe('React', () => {

expect(childCalls).toEqual([
['a', 'a'],
['a', 'ac'],
['ac', 'ac'], // then store update is processed
['ac', 'acb'],
['acb', 'acb'], // then store update is processed
['acb', 'acbd'],
['acbd', 'acbd'] // then store update is processed
])
expect(childMapStateInvokes).toBe(4)
expect(childMapStateInvokes).toBe(7)
})

it('works in <StrictMode> without warnings (React 16.3+)', () => {
Expand Down
14 changes: 7 additions & 7 deletions test/components/connect.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1858,7 +1858,6 @@ describe('React', () => {
}

@connect((state, props) => {
expect(props.count).toBe(state)
return { count: state * 10 + props.count }
})
class C extends React.Component {
Expand Down Expand Up @@ -2989,8 +2988,6 @@ describe('React', () => {
@connect((state, parentProps) => {
childMapStateInvokes++
childCalls.push([state, parentProps.parentState])
// The state from parent props should always be consistent with the current state
expect(state).toEqual(parentProps.parentState)
return {}
})
class ChildContainer extends Component {
Expand All @@ -3011,23 +3008,26 @@ describe('React', () => {
rtl.act(() => {
store.dispatch({ type: 'APPEND', body: 'c' })
})
expect(childMapStateInvokes).toBe(2)
expect(childCalls).toEqual([['a', 'a'], ['ac', 'ac']])
expect(childMapStateInvokes).toBe(3)
expect(childCalls).toEqual([['a', 'a'], ['a', 'ac'], ['ac', 'ac']])

// setState calls DOM handlers are batched
const button = tester.getByText('change')
rtl.fireEvent.click(button)
expect(childMapStateInvokes).toBe(3)
expect(childMapStateInvokes).toBe(5)

rtl.act(() => {
store.dispatch({ type: 'APPEND', body: 'd' })
})

expect(childMapStateInvokes).toBe(4)
expect(childMapStateInvokes).toBe(7)
expect(childCalls).toEqual([
['a', 'a'],
['a', 'ac'],
['ac', 'ac'],
['ac', 'acb'],
['acb', 'acb'],
['acb', 'acbd'],
['acbd', 'acbd']
])
})
Expand Down
8 changes: 3 additions & 5 deletions test/hooks/useSelector.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,13 +350,11 @@ describe('React', () => {
}
)

let sawInconsistentState = false
let lastRenderInconsistentState = false

const Child = ({ parentCount }) => {
const result = useSelector(({ count }) => {
if (count !== parentCount) {
sawInconsistentState = true
}
lastRenderInconsistentState = count !== parentCount
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this change. The whole purpose of this test was to ensure the selector never sees inconsistent state, not just on the latest render.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we NO more read the last state in the render phase, the "parent-child-issue" is resolved with 2 render, as every change of the store make 2 event-callback one on the parent and one on the child.
Before the issue was resolved with the "hack" of reading always the fresh Store state, even if we are on the "preview" scheduled refresh came out from "checkForUpdate" callback.

That hack while resolve the "parent-issue" with 1 render less, broke the changes normal queue on the the componente, that produce tearing on CM where every component could rendered in a random way, jumping the "changhes queue"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please try the demo of @dai-shi with my version and with old one. you will see the differences on a fast store changes, with slow render.


return count + parentCount
})
Expand All @@ -372,7 +370,7 @@ describe('React', () => {

store.dispatch({ type: '' })

expect(sawInconsistentState).toBe(false)
expect(lastRenderInconsistentState).toBe(false)

spy.mockRestore()
})
Expand Down
13 changes: 7 additions & 6 deletions test/react-native/batch-integration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,6 @@ describe('React Native', () => {
@connect((state, parentProps) => {
childMapStateInvokes++
childCalls.push([state, parentProps.parentState])
// The state from parent props should always be consistent with the current state
expect(state).toEqual(parentProps.parentState)
return {}
})
class ChildContainer extends Component {
Expand All @@ -112,23 +110,26 @@ describe('React Native', () => {
rtl.act(() => {
store.dispatch({ type: 'APPEND', body: 'c' })
})
expect(childMapStateInvokes).toBe(2)
expect(childCalls).toEqual([['a', 'a'], ['ac', 'ac']])
expect(childMapStateInvokes).toBe(3)
expect(childCalls).toEqual([['a', 'a'], ['a', 'ac'], ['ac', 'ac']])

// setState calls DOM handlers are batched
const button = tester.getByTestId('change-button')
rtl.fireEvent.press(button)
expect(childMapStateInvokes).toBe(3)
expect(childMapStateInvokes).toBe(5)

rtl.act(() => {
store.dispatch({ type: 'APPEND', body: 'd' })
})

expect(childMapStateInvokes).toBe(4)
expect(childMapStateInvokes).toBe(7)
expect(childCalls).toEqual([
['a', 'a'],
['a', 'ac'],
['ac', 'ac'],
['ac', 'acb'],
['acb', 'acb'],
['acb', 'acbd'],
['acbd', 'acbd']
])
})
Expand Down