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

Jump to bottom

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 1 commit
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
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
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