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 5 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
15 changes: 9 additions & 6 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,8 @@ function useSelectorWithStoreAndSubscription(
store,
contextSub
) {
const [, forceRender] = useReducer(s => s + 1, 0)
const [reduxState, forceRender] = useState(store.getState())
const latestReduxState = useRef()

const subscription = useMemo(() => new Subscription(store, contextSub), [
store,
Expand All @@ -31,7 +32,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(latestReduxState.current || reduxState)
timdorr marked this conversation as resolved.
Show resolved Hide resolved
} else {
selectedState = latestSelectedState.current
}
Expand All @@ -53,10 +54,12 @@ 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)) {
latestReduxState.current = newReduxState
return
}

Expand All @@ -68,8 +71,8 @@ function useSelectorWithStoreAndSubscription(
// changed
latestSubscriptionCallbackError.current = err
}

forceRender({})
latestReduxState.current = undefined
forceRender(newReduxState)
timdorr marked this conversation as resolved.
Show resolved Hide resolved
}

subscription.onStateChange = checkForUpdates
Expand Down
44 changes: 44 additions & 0 deletions test/cm/.eslintrc.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
{
"plugins": [
"react-hooks"
],
"extends": [
"airbnb"
],
"env": {
"browser": true
},
"settings": {
"import/resolver": {
"node": {
"extensions": [".js", ".ts", ".tsx"]
}
}
},
"rules": {
"react-hooks/rules-of-hooks": "error",
"react-hooks/exhaustive-deps": "error",
"react/jsx-filename-extension": ["error", { "extensions": [".js"] }],
"react/prop-types": "off",
"react/jsx-one-expression-per-line": "off",
"import/prefer-default-export": "off",
"no-param-reassign": "off",
"default-case": "off",
"arrow-body-style": "off",
"no-plusplus": "off",
"import/no-useless-path-segments": 0,
"no-console": "off",
"no-await-in-loop": "off",
"arrow-parens": ["error", "as-needed", { "requireForBlockBody": true }],
"react/jsx-fragments": "off"
},
"overrides": [{
"files": ["__tests__/**/*"],
"env": {
"jest": true
},
"rules": {
"import/no-extraneous-dependencies": ["error", { "devDependencies": true }]
}
}]
}
4 changes: 4 additions & 0 deletions test/cm/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
*~
*.swp
node_modules
/dist
4 changes: 4 additions & 0 deletions test/cm/.travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
language: node_js
node_js:
- 8
- 10
Loading