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 all 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
37 changes: 16 additions & 21 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({ reduxState: store.getState() })

// Propagate any mapState/mapDispatch errors upwards
if (previousStateUpdateResult && previousStateUpdateResult.error) {
Expand All @@ -261,12 +253,11 @@ export default function connectAdvanced(
return childPropsFromStoreUpdate.current
}

// TODO We're reading the store directly in render() here. Bad idea?
// 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(
previousStateUpdateResult.reduxState,
wrapperProps
)
}, [previousStateUpdateResult.reduxState, 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 +313,12 @@ export default function connectAdvanced(

// If the child props haven't changed, nothing to do here - cascade the subscription update
if (newChildProps === lastChildProps.current) {
forceComponentUpdateDispatch(prev => {
prev.error = error
prev.reduxState = latestStoreState
return prev
})

if (!renderIsScheduled.current) {
notifyNestedSubs()
}
Expand All @@ -336,10 +333,8 @@ export default function connectAdvanced(

// If the child props _did_ change (or we caught an error), this wrapper component needs to re-render
forceComponentUpdateDispatch({
type: 'STORE_UPDATED',
payload: {
error
}
error,
reduxState: latestStoreState
})
}
}
Expand Down
16 changes: 10 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,7 @@ function useSelectorWithStoreAndSubscription(
store,
contextSub
) {
const [, forceRender] = useReducer(s => s + 1, 0)
const [reduxState, setReduxState] = useState({ value: 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.value)
} else {
selectedState = latestSelectedState.current
}
Expand All @@ -53,10 +53,15 @@ 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)) {
setReduxState(prev => {
prev.value = newReduxState
Copy link
Contributor

Choose a reason for hiding this comment

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

@salvoravida As I said in other thread, I'm not confident this works for the future. Do you ever feel like just trying my previous snippet???

Copy link
Author

@salvoravida salvoravida Nov 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

For what i have understood, useRef is not CM safe. its a shared mem between branches. we need an useStateRef, a way to save data to current branch states queue.
I think it is safe, just says React to not re-render, but store a value on that branch.

i dont' think that reading value from the future is ok in CM and event streaming, but i will try your idea asap.
Have you tried it?

Copy link
Contributor

Choose a reason for hiding this comment

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

This line is mutating the existing state value, though, which is definitely not CM-safe.

Copy link
Author

Choose a reason for hiding this comment

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

no, because it is executed only on next update, before next render.

Copy link
Author

@salvoravida salvoravida Nov 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

on every branch, so every branch has it's own reduxState queue changes. this is why there is NO more tearing, even if with last test, that simulate this edge case.

Copy link

Choose a reason for hiding this comment

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

hm, so it's more or less like...:

const [refState, forceUpdateRef] = React.useState({ current: null })
React.useImperativeHandle(refState, () => 'some value', [ ...optional_deps ])

Isn't this just like a short cut?, I mean, if keeping state in ref and just using forceUpdate. If it's within a transition, forceUpdate will be scheduled for later but the ref will still change which I think this does. Using useState for refs is just like merging a ref with forceUpdate into one line of code:

const ref = useRef()
const forceUpdate = useReducer(c => c + 1, 0)[1]
somewhere {
   ref.current = someValue
   forceUpdate()
}
...
const [ref, forceUpdate] = useState({ current: null })
somewhere {
   ref.current = someValue
   forceUpdate({ ...ref })
}

Both forceUpdate will schedule "for later" if within transition.

Copy link
Author

Choose a reason for hiding this comment

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

Yes but using ref, another rendering brahch could update the same value before or after, like shared mem between 3d. Instead states ref are duplicated on every branch and every state ref updater will work on its own branch rendering.

Copy link

@eddyw eddyw Nov 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Idk, this is how I see it:

queue no transition => [
  setReduxState(p => { p.value = reduxState 1; return p }),
  setReduxState(p => { p.value = reduxState 2; return p }), // before p.value is reduxState 1
  setReduxState(p => { p.value = reduxState 3; return p }), // before p.value is reduxState 2
]
start transition queue => [
  later setReduxState(p => { p.value = reduxState 4; return p }), // previous 'p' is unknown
]
queue outside transition => [
  setReduxState(p => { p.value = reduxState 5; return p }), // before p.value is reduxState 3
]
stop transition queue => [
  run previous:
    initial state: reduxState 5 <<<< this is previous 'p' now
    setReduxState(p => { p.value = reduxState 4; return p }), // <<< inconsistent state
]

Now, current @dai-shi tests using transition are not exactly right. It's testing dispatching in transition.. but, since there is no Suspense, no component is waiting for anything, so there is no need to queue for later.

Sometimes there may be some flickering on screen, when you use transition but nothing causes interrupting of rendering (like throw promise on Suspense), I've seen on some demos I was testing with, I think this is a bug related to what Dan mentioned on twitter, maybe? Point is, you can't test using transition without causing interrupt on render (throw on Suspense), otherwise it is pointless to wrap it in startTransition.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not following the discussion in deep, but:

without causing interrupt on render (throw on Suspense)

My understanding is a bit different. We can cause interrupt with heavy computation without throwing promise. That's the whole point of my tests.

P.S. Did I ever test using transition? Just today, I made new checks using useTransition based on @eddyw 's demo. No Suspense. No throwing promises. Is it pointless?

Copy link

Choose a reason for hiding this comment

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

Never mind, ignore the above 🙈
I've copied the tests over this PR locally which did have useTransition but you finished committing later, so I had them broken

return prev
})
return
}

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

forceRender({})
setReduxState(() => ({ value: newReduxState }))
}

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