reduxjs / react-redux

Official React bindings for Redux
https://react-redux.js.org
MIT License
23.36k stars 3.37k forks source link

Concurrent Mode #1351

Closed brunolemos closed 2 years ago

brunolemos commented 5 years ago

Do you want to request a feature or report a bug?

Feature

What is the expected behavior?

Full compatibility with Concurrent Mode

Details

We should be close to a Concurrent Mode release, and people are already experimenting with it on their projects. I am not sure but it seems react-redux is not compatible yet, based on this tweet and this pr.

markerikson commented 5 years ago

There's still no ETA on when Concurrent Mode will be released, and there's also a general lack of clarity on what exactly it will include, what the impacts are, and what the points of incompatibility may be.

As Dan said in issue #1164:

We need to be clear that there are several β€œlevels” of compatibility with new features of React. They’re not formalized anywhere yet but a rough sketch for a library or technique X could be:

  • X breaks in sync mode
  • X works in sync mode but breaks in concurrent mode
  • X works in concurrent mode but limits its DX and UX benefits for the whole app
  • X works in concurrent mode but limits its UX benefits for the whole app
  • X works in concurrent mode but limits its DX and UX benefits for features written in X
  • X works in concurrent mode but limits its UX benefits for features written in X
  • X works in concurrent mode and lets its users take full advantage of its benefits

This is not a strict progression and there’s a spectrum of tradeoffs. (For example, maybe there is some temporary visual inconsistencies such as different like counts, but no crashes are guaranteed.)

But we need to be more precise about where React Redux is, and where it aims to be.

Until Concurrent Mode is actually out and completely documented, we can really only speculate on the potential interactions, and there's a lot of other stuff that I'd rather spend my time worrying about.

I'll leave this open as a placeholder, but it's not an immediate priority.

dai-shi commented 5 years ago

I don't mean any rush, but while it's not an immediate priority, let's do some experiment. I made a small repo for reproduction of "tearing" (as far as I understand). https://github.com/dai-shi/will-this-react-global-state-work-in-concurrent-mode

It would be nice if someone could review it as I might be biased. Please file issues there about this tool or any related discussion, not to disturb this issue here.

gcloeval commented 5 years ago

Hello redux team,

We were just wondering if there are any updated guidelines or at least high-level predictions on building components libraries on top of react-redux and making sure, to the best knowledge available, they will play nicely with concurrent mode?

We have switched fully to hooks version with useSelector for reading data and useDispatch for writing. We are also making sure that we do not do any side effects during rendering, i.e. all the dispatches are done inside useEffect. To be honest - just this - to only dispatch inside useEffect - has resulted in complete refactoring of entire codebase. Since its been a few months that this issue was opened, it would be great to get some more feedback from the team about the future.

Thanks for your hard work!

markerikson commented 5 years ago

@gcloeval: nope, no further updates, because there's no new info available on Concurrent Mode.

Realistically, I expect React-Redux won't be "compatible", in the sense that folks expect store updates to be readable immediately. That said, it may not be a blocker from using Concurrent Mode.

Guria commented 4 years ago

@markerikson now we have updates on concurrent mode could we see a future of this?

markerikson commented 4 years ago

The short version is that it's going to be up to the community to experiment and see exactly where the incompatibilities are. I don't currently have time to look into it myself, and I don't have enough experience doing data fetching to work with most of the concepts that are described.

As an example, Dan told me that Suspense transitions would likely be problematic, because you'd end up with cases where the Redux store is already updated from a dispatched action and thus trying to display "new" UI, whereas Suspense would want to still be displaying "old" UI.

Dan gave some short related answers on Redux + Suspense/CM in this SO answer.

The other major issue is "tearing", where new Redux actions might be dispatched during the pauses in rendering, and thus cause different parts of the tree to read different versions of the store state during the same render pass. (The v6 implementation was intended to avoid this by passing the current store state via context for consistency, but we had to drop that approach and go back to direct subscriptions due to perf problems.)

Dan has strongly recommended trying to duplicate some of the Suspense/CM demos with Redux. Someone on Reactiflux was trying to do a bit of that the other day, and I'll see if I can paste in their comments.

But yeah, I'm gonna have to ask folks to go experiment with this and report their findings here. My own focus for the foreseeable future is on rewriting the Redux core docs and encouraging people to use Redux Starter Kit.

salvoravida commented 4 years ago

Hi folks. My idea is to change forceRender into useSelector hook, let instead set a state, and return that state. This should transform redux state changes into react useState changes that should batched and eventually paused form react concurrent mode useTransition.

markerikson commented 4 years ago

@salvoravida : if you've got ideas, please feel free to prototype them and see how they work out.

That said, I'm not sure how what you're describing is actually different than what we do right now internally, and it doesn't change the issue that there's going to be differences between what the Redux store says is the "current" state value, and what React is currently wanting to display based on Suspense changes.

eddyw commented 4 years ago

The "tearing" issue is .. expected. At least within Suspense.

If the old UI needs to display "current" data, then it's a blocking-update and React will warn about it with or without using Redux. So the point is to "bake" useTransition within your components (as suggested in docs). Within a transition, it basically tells React to update the new UI (that is in memory and not displayed yet).

The mechanism is pretty simple actually. This is what tells Suspense to .. suspend the UI:

const MyComponent = () => throw { then(tellSuspenseThatWeAreReadyForNewUI, fail) {} }
const App = () => {
  return (
    <Suspense fallback="New UI not ready">
       <MyComponent />
    </Suspense>
  )
}

So, the main issue I think is .. what to throw πŸ™ˆ and how/if react-redux can help with that or if it needs to be implemented on user side.

This is a very naive implementation:

const promise = { // throw this instead
  then(tellSuspenceToContinue, throwAnError) {
    const unsubscribe = store.subscribe(() => {
      if (store.getState() !== STATUS.PENDING) {
        ubsubscribe();
        tellSuspenceToContinue();
      }
    });
  }
};

Here is a sandbox which describes what I mentioned above: https://codesandbox.io/s/peaceful-spence-yh8w7 It shows the differences with/without useTransition (check console)

A complete separate issue is tearing that is not related to "intentional transition/suspense tearing" that could happen when nested components render too slow. However, I don't know if this is a bug or not. I found out that some slow renders are never committed to the DOM, so I'm just waiting for a more .. stable - experimental release πŸ™ˆ to know if it's really a thing to worry about or not. Currently happens because of nested Providers with react-redux. However, I haven't tried using react-redux hooks. I suspect that the issue shouldn't be if there are no connected components and just hooks but I haven't really had the time to test that.

markerikson commented 4 years ago

Related issue: https://github.com/JustSift/ReSift/issues/32

salvoravida commented 4 years ago

@eddyw i can't see any unexpected behavior in your demo.

on StoreState changes -> useState.setState within startTransition setState delay 5000ms so you will see correctly old UI until "Loading". that's the correct behavior.

if after startTransition, storeState changes many times it will enqueue setState many times. after transition end due to 5000ms timeout or promiseResolve (Suspense end transition on promise resolve) all pending setStates will be batched/executed and you will see on screen only last "reduxStore" update.

that's fine.

Am i missing something?

markerikson commented 4 years ago

We're having a good discussion with Dan atm on Twitter about whether it would be helpful for the React team to try porting a couple of the Suspense demos to Redux to show what's broken, and and how we might be able to understand the issues involved.

A few entry points into the subthreads (read up and down from these links):

https://twitter.com/dan_abramov/status/1192569023529140231 https://twitter.com/dan_abramov/status/1192570534543933441 https://twitter.com/EphemeralCircle/status/1192574816815108096 https://twitter.com/dan_abramov/status/1192579208423378946

dai-shi commented 4 years ago

Oh, there's so much going on here.

In my perspective, the tearing issue is relatively easy. We just take the approach either by use-subscription or reactive-react-redux. (Both have slight issues, but let's put aside.)

What's hard is the new pattern with Suspense and CM. Transitions and Render-as-You-Fetch pattern. This is not because of the core libraries redux and react-redux, but rather async middleware and best practices. In my blog post, I used "action hooks". My optimistic expectation is reactive-react-redux with no async middleware may work in CM. The question is if you call it a "React Redux" app, I mean what value do we get from Redux.

salvoravida commented 4 years ago

hum i think we are mixing many points together. Suspense "loading states" capturing via throw promise it has nothing to do with react-redux.

instead startTransition that will stop low priority states commit will work perfectly with redux, as @eddyw demos shows.

dai-shi commented 4 years ago

I'm not sure if I'm on the same track, but what I'm suggesting is the pattern that Redux doesn't handle async functions including loading state. I'm not sure if that pattern is acceptable in the React Redux community, so that's my question. Yeah, this topic might be too broad.


Here's a tiny example I have in mind. https://codesandbox.io/s/test-redux-in-concurrent-mode-du6ln Note: It's violating the Redux convention: state should be serializable. Note2: I'm using my experimental library, but it's not very important. It's just creating an object that throws.

On the second thought, this example can probably be implemented with redux-thunk.

salvoravida commented 4 years ago

I don't mean any rush, but while it's not an immediate priority, let's do some experiment. I made a small repo for reproduction of "tearing" (as far as I understand). https://github.com/dai-shi/will-this-react-global-state-work-in-concurrent-mode

It would be nice if someone could review it as I might be biased. Please file issues there about this tool or any related discussion, not to disturb this issue here.

hi, i have done some test with your demos, here are my 2 cents:

import React from 'react';
import { createStore } from 'redux';
import { Provider, useSelector, useDispatch } from 'react-redux';
import { unstable_runWithPriority, unstable_UserBlockingPriority } from 'scheduler';

import {
  syncBlock,
  useRegisterIncrementDispatcher,
  reducer,
  ids,
  useCheckTearing,
} from '../common';

const store = createStore(reducer);

const Counter = React.memo(() => {
  const count = useSelector(state => state.count);
  syncBlock();
  return <div className="count">{count}</div>;
});

const Main = () => {
  const dispatch = useDispatch();
  const count = useSelector(state => state.count);
  useCheckTearing();
  useRegisterIncrementDispatcher(React.useCallback(() => {
    unstable_runWithPriority(unstable_UserBlockingPriority, () => {
      dispatch({ type: 'increment' });
    });
  }, [dispatch]));
  const [localCount, localIncrement] = React.useReducer(c => c + 1, 0);
  return (
    <div>
      <button type="button" id="localIncrement" onClick={() => dispatch({ type: 'increment' })}>Increment remote from app</button>
      <h1>Remote Count</h1>
      {ids.map(id => <Counter key={id} />)}
      <div className="count">{count}</div>
      <h1>Local Count</h1>
      {localCount}
      <button type="button" id="localIncrement" onClick={localIncrement}>Increment local count</button>
    </div>
  );
};

const App = () => (
  <Provider store={store}>
    <Main />
  </Provider>
);

export default App;

1) button onclick outside a ReactApp has lowpriority so dispatch-> "50 setStates" are "streamed" in more workInProgress Update and the "tearing" effect is Perfectly correct! CM "streams" low priority update like. the misundestandig about your demo is that an externa button is not "remote" low priorty work Infact a) wrapping around high priority userBlocking your external button there is NO more tearing. b) putting the button inside react app, react engine execute with userBlocking priority the "onClick" event and there is NO tearing.

2) your "reactive-react-redux" is not so "reactive" as it ALWAYS propagate reduxState via Context, that is EQUIVALENT tu dispatch always in userBlockingPriority (update 50 counter states at the same time) with the consequence that you will see jumps in the counter on screen when click fast.

so i think that tearing is not a bug, is the consequence of "streaming" updates from a "remote"

and react-redux is capable of that with every priority needed.

salvoravida commented 4 years ago

so if folks want IMMEDIATE update from everywhere then

function useImmediateDispatch() {
  const dispatch = useDispatch();
  return (...args) => {
    unstable_runWithPriority(unstable_UserBlockingPriority, () => {
      dispatch(...args);
    });
  };
}
salvoravida commented 4 years ago

removed.

eddyw commented 4 years ago

@salvoravida my demo just demonstrates what I was talking about. The "intentional tearing" caused by useTransition and what blocking updates look like. One issue I mentioned is "what to throw" which basically just means, how Redux can tell React to wait. From what I see this "waiting promise-like thing" should be a subscription itself?

Btw, the demo just works because state is actually in React state. If you replace these lines:

      <pre>
        <code>{JSON.stringify(store.getState(), null, 2)}</code>
      </pre>

with:

      <pre>
        <code>{JSON.stringify(state, null, 2)}</code>
      </pre>

This is outside of Suspense and .. the broken behavior? or what React would expect the state to look like in there.

@markerikson IMHO, from what I read ("Redux is broken") from Dan and what's on React docs, it kind of seems like they just want to say "Lift your state to where it matters instead of having a global store as React has been suggesting for years in their docs" or, let's just say it, ... multiple stores which is actually the reality (you can count all useState and useReducer as stores), so yeah ... ~and "use Relay" is silently mentioned everywhere~ πŸ™ˆ

Anyways, I made a couple of codesandbox: https://codesandbox.io/s/test-redux-in-concurrent-mode-with-react-redux-gj63q This is with redux & react-redux. As Dan said in StackOverflow, wrapping with startTransition doesn't work

https://codesandbox.io/s/test-react-redux-in-concurrent-mode-without-redux-foxxo This is without Redux but with react-redux. It's a naive implementation of Redux using useReducer. This is the ""expected"" behavior. If you click on "start" and then type something in the input (which also dispatches an action), you'd see that the suspended state wasn't merged yet as long as the transition lasts (timeoutMs). Ignore the warning πŸ˜…, that's another thing to worry about.

I think we need a concurrent Redux πŸ™ˆ

salvoravida commented 4 years ago

Hi folks. My idea is to change forceRender into useSelector hook, let instead set a state, and return that state. This should transform redux state changes into react useState changes that should batched and eventually paused form react concurrent mode

In this pr https://github.com/reduxjs/react-redux/pull/1455 i fixed the error of reading store state into render phase.

@dai-shi this fix tearing WITHOUT any priority wrapper even with with "your external buttton"

i will do a codesanbox with your demo and my fix.

The mental modal is -> store Changes -> subcription call -> enqueque storeState, NOT forceRe-render and read store state on render phase. (that may be discarded or delayed)

salvoravida commented 4 years ago

@eddyw throing promise to "comuncate" with Suspense si not releated with tearing, that i have fixed in upside PR.

i will investigate more on your demo as soon as possible, with new react-redux.

salvoravida commented 4 years ago

here is a temp build if you want to try in CM useSelector "@salvoravida/react-redux": "^7.1.4",

Regards!

salvoravida commented 4 years ago

@eddyw https://codesandbox.io/s/test-redux-in-concurrent-mode-with-react-redux-pytbh

HERE is your demo "Test Redux in Concurrent mode (with React-Redux)" with import { Provider, useSelector } from "@salvoravida/react-redux";

πŸŽ‰ THAT IS 100% working with useTransition !!

salvoravida commented 4 years ago

Finally

The mental modal is -> store Changes -> subcriptions call -> enqueque storeState, NOT forceRender

Enjoy Redux & React-redux 100% CM compatible.

Community will save you, little child. Do not worry.

dai-shi commented 4 years ago

@eddyw

The "intentional tearing" caused by useTransition and what blocking updates look like.

I guess you already know it, but this "intentional tearing" is different from what I originally talked about, which is tearing in multiple components subscribing separately to a redux store. See this for more information.

We need better wording...

eddyw commented 4 years ago

@salvoravida πŸ‘ cool cool! ... but πŸ™ˆ

enqueque storeState, NOT forceRender

wrap within React.memo, nothing will happen if within transition πŸ™ˆ https://codesandbox.io/s/test-redux-in-concurrent-mode-with-react-redux-salvoravidareact-redux-j2e17

A separate thing:

Warning: App triggered a user-blocking update that suspended.

The fix is to split the update into multiple parts: a user-blocking update to provide immediate feedback, and another update that triggers the bulk of the changes.

Refer to the documentation for useTransition to learn how to implement this pattern.

If not wrapping within React.memo, it works, but this warning still happens if typing on the input. Technically, none of the components within Suspense should re-render or update because they don't use that "slice" of state.. I think

dai-shi commented 4 years ago

I would suggest to split the discussion:

X works in concurrent mode but limits its UX benefits for the whole app

Isssue1: Existing React Redux apps that run in SyncMode should run in Concurrent Mode (createRoot)

X works in concurrent mode but limits its UX benefits for features written in X

Issue2: Existing React Redux apps to get benefit of concurrent rendering (priority)

X works in concurrent mode and lets its users take full advantage of its benefits

Issue3: What would Suspense-oriented React Redux apps look like and how a library should support new best practices

salvoravida commented 4 years ago

@eddyw

See #1351 (comment) If component is wrapped within React.memo, it doesn't work πŸ˜›

here is the fix πŸ˜›πŸ˜›πŸ˜› https://codesandbox.io/s/test-redux-in-concurrent-mode-with-react-redux-salvoravidareact-redux-9jo9z

but please note that is a bug of the current "experimental react" with "SimpleMemoComponent" infact force memo Non simple it works.

react-dom-internal

function updateMemoComponent(current$$1, workInProgress, Component, nextProps, updateExpirationTime, renderExpirationTime) {
  if (current$$1 === null) {
    var type = Component.type;

    if (isSimpleFunctionComponent(type) && Component.compare === null && // SimpleMemoComponent codepath doesn't resolve outer props either.
    Component.defaultProps === undefined) {
      var resolvedType = type;

      {
        resolvedType = resolveFunctionForHotReloading(type);
      } // If this is a plain function component without default props,
      // and with only the default shallow comparison, we upgrade it
      // to a SimpleMemoComponent to allow fast path updates.

      workInProgress.tag = SimpleMemoComponent;
      workInProgress.type = resolvedType;

      {
        validateFunctionComponentInDev(workInProgress, type);
      }

      return updateSimpleMemoComponent(current$$1, workInProgress, resolvedType, nextProps, updateExpirationTime, renderExpirationTime);
    }

i will open a issue on React.

So this "issue" is not related to useSelector! πŸŽ‰

salvoravida commented 4 years ago

and here is: https://github.com/facebook/react/issues/17318

eddyw commented 4 years ago

@salvoravida interesting ... So, I just set defaultProps to continue playing πŸ™ˆ I implemented a very naive "Reducer suspense" which basically delays the reducer from setting its state until it's no longer in the transition: https://codesandbox.io/s/test-redux-in-concurrent-mode-with-react-redux-salvoravidareact-redux-1xpwn

Seems to work well .. but I stumbled across a weird behavior. Typing in both inputs (one inside, another outside Suspense) dispatches the same action. This action is handled by two reducers, so the same text is set within posts.text and input.text (this was just to test that posts reducer state could be suspended). What is weird.., if you type in the second input (within Suspense), the first one doesn't get updated. In console, you can see that it is in fact updated in the Redux store. I don't know if this is an issue with useSelector or not. I really can't make sense of it.

salvoravida commented 4 years ago

@eddyw i dont think that reducers are the place where throw anything! reducers and redux must be pure.

i'm working on some different point to implement thrown from data coming from redux, but it will involve saga and loading states. it has nothign to do with useSelector and react-redux.

eddyw commented 4 years ago

@salvoravida yes, it's just a naive implementation Btw, it is a bug in useSelector. I had this: const text = useSelector(s => s.input.text); However, this was suppose to be: const text = useSelector(s => s.input); No error or warning thrown/shown on console. :/

Edit: yeah, no bug lol it's my fault. Basically, s.input = '' so this is perfectly valid s.input.text => undefined

salvoravida commented 4 years ago

CM is so amazing!πŸŽ‰

Fast low priority updates, will be batched, and ignored, with many less render!

CM "fast" dispatch

cm1

Sync mode dispatch

cm2

salvoravida commented 4 years ago

@eddyw πŸŽ‰

PASS  __tests__/all_spec.js (12.341s)
  react-redux
    √ check1: updated properly (8053ms)
    √ check2: no tearing during update (1ms)
    √ check3: ability to interrupt render
    √ check4: proper update after interrupt (2108ms)

Test Suites: 1 passed, 1 total
Tests:       4 passed, 4 total
MargaretKrutikova commented 4 years ago

Has anyone looked into use-subscription hook? It was created at Facebook and is designed to safely manage subscriptions in concurrent mode.

I implemented hooks support in a redux-like solution for reason-react (reductive) using use-subscription. My first attempt was basically porting the current code for hooks in react-redux into reasonml, but then after long discussions we went with a slightly different approach with use-subscription.

Maybe it is something that can be used in react-redux? Appreciate any thoughts! πŸ˜„

markerikson commented 4 years ago

@MargaretKrutikova : yeah, we'd based several aspects of our original implementation on that. Not sure how much we've deviated from at by now.

ricokahler commented 4 years ago

Hey all πŸ‘‹,

I'm very excited about concurrent and suspense! Redux + React Redux are libraries we in production and it would definitely be a blocker for adoption if it didn't support concurrent mode etc.

I'm relatively new to this but I'm ready and excited to experiment and problem solve. I would like to suggest one starting topic of discussion to onboard myself and anyone else who is excited to experiment and try these features out:

What does supporting concurrent mode even mean?

This is very related to what @markerikson posted back in July quoting Dan.

We need to be clear that there are several β€œlevels” of compatibility with new features of React. They’re not formalized anywhere yet but a rough sketch for a library or technique X could be:

X breaks in sync mode X works in sync mode but breaks in concurrent mode X works in concurrent mode but limits its DX and UX benefits for the whole app X works in concurrent mode but limits its UX benefits for the whole app X works in concurrent mode but limits its DX and UX benefits for features written in X X works in concurrent mode but limits its UX benefits for features written in X X works in concurrent mode and lets its users take full advantage of its benefits This is not a strict progression and there’s a spectrum of tradeoffs. (For example, maybe there is some temporary visual inconsistencies such as different like counts, but no crashes are guaranteed.)

But we need to be more precise about where React Redux is, and where it aims to be.

I don't really need to answers for these in particular but I really just want to define what "supporting concurrent mode" means for Redux.

Here are my questions that would help me contribute:

  1. How does Redux/React-Redux not support concurrent mode today? Is this just because of tearing?
  2. Are there any definitions for concurrent-mode safe vs full concurrent mode support?
  3. If so can we define what "concurrent-mode safe" and "full concurrent mode support" means for React-Redux?
    1. Does CM safe mean no tearing issues? Does full CM support just mean making it work transitions (i.e. useTransition and useDeferredValue)? Does it mean more?
markerikson commented 4 years ago

@ricokahler : yes, exactly!

The PR at #1455 looks like it's worth investigating further, but overall I would like to collect a much more detailed list of exactly what points of compat/incompat we are dealing with, figure out which of those are resolvable by lib changes if any, and formally document things.

eddyw commented 4 years ago

This is from my experience and understanding from testing CM:

  1. The tearing issue is described here by @markerikson -> https://stackoverflow.com/questions/54891675/what-is-tearing-in-the-context-of-the-react-redux This is what the mentioned PR is trying to solve. That should make existing RR apps just work in CM (consistent rendering). Let's call it "concurrent-mode safe"

  2. What doesn't work? I made this small demo: https://codesandbox.io/s/cm-simple-test-x5umg This doesn't use Redux or React-Redux. Test this in the following ways:

    • First test: click "Start Suspense", then click several times "Increment with Transition", then "Stop Suspense" You'll see that while you were incrementing, nothing was displayed on screen and this update happened on a "separate branch" of the state in memory until you stopped the transition.

The issue is, Redux is not able to create a "branch" of the state and update it in memory. Every action results in the whole store being updated, regardless of using transition or not. Now, React-Redux does keep the slice of the state you subscribed to in React state, so technically, it's possible to dispatch an action wrapped within a transition, since this is sync, it'd result in a state update within a transition and React will know to update this forked state. However, this behaves differently if you update the same slice of state with & without transition while a component is suspended. This is the same demo but with Redux & React-Redux with @salvoravida changes: https://codesandbox.io/s/cm-simple-test-tx34i

Do the "second test" described above and compare what happens. So, it's not possible to "fix" this behavior because .. well, we can't. Redux just doesn't have a way to do branches (like git branches) of state since it isn't even aware of React.

Another "issue" is .. how to notify Suspense that we have the data ready? In the demo, you can see an example of how it can be done, but there isn't a guide of how to do it since this depends on your own implementation. React docs use wrapPromise which return .read() which either returns the value or throws. This should probably be implemented on user-side since there isn't a general way of doing it that will cover all user cases.

So, to be CM compatible, you need to know which actions you dispatch are wrapped within transition and make sure no other action modifies the slice of state that the transition depends on or it will cause a blocking-update. Maybe this could be baked within you action creators:

const fetchUserData = (isTransition) => ({ type: 'FETCH', isTransition })

And you could let the reducer, that handles that slice, know that the action dispatched was within a transition. Your reducer could then prevent any further action that is dispatched outside of a transition like this:

const reducer = (state, action) => {
   if (state.isTransition && !action.isTransition && action.type !== 'STOP_FETCH') {
      console.warn('Action dispatched while state is in transition')
      return state
   }
}

Point is, the lib can "help" but not solve all the issues. The tearing issue can be solve by the lib, but handling "Suspense" (what you "throw") and accidental blocking-update should probably be handled by user-side. The simple rule is, if your state was updated within "transition", then every other dispatched action should be within transition until .. quit the Suspense waiting state (if there is a better term for that)

However, that will just make it as close as possible to full CM support. It will work with Suspense and useTransition, but .., since there isn't a concept of branching states in Redux, any other part of the app that is subscribed to the state that is suppose to be "updated in memory but not displayed yet" could potentially still display the very last Redux state. You can see it here: https://codesandbox.io/s/cm-simple-test-b1sqq

You'll see that the input label got updated to display the latest value only when you typed and not when the store updated. I can see that here is where people could get really confused thinking that RR doesn't work but .. this is expected since the store has only one version. The reason why it didn't update when store changed is because the selector value was got within a transition, so the Input component bails out of rendering, however, typing on the input causes the component to re-render which shows the latest selected value. With React state only, this value wouldn't update and typing on the input wouldn't display the "other branch"

For this last issue, could the lib help? I really don't know. Maybe if RR were to provide its own useTransition hook, then RR would be aware of which dispatched actions should cause a "merged store"? or maybe this just shouldn't be the job of RR to solve, maybe it could be solved with a special store enhancer or middleware for Redux? Or should we just write it as "best practices" not to subscribe to slice of state that will be used within "Suspense"?

markerikson commented 4 years ago

@eddyw : yeah, the issue you're describing with "lack of state branching" is something Dan was specifically pinpointing.

I've had very vague ideas of some kind of store enhancer that could juggle in-flight actions, wait for React to actually be ready to commit them, and then finally apply them to the Redux store, but tbh it seems like a horribly complex idea that probably wouldn't work out well.

If you look back in the very early "async React" issues from last year (like #890 ), Dan and Andrew were suggesting hypothetically creating an API-compatible "Redux store" on top of React state, to allow React to control the queueing of state updates. Unfortunately, I think that's also a non-starter, because there's a lot of need to interact with the store outside the React component tree, and I think that the perf behavior would be significantly worse overall based on what I've seen from v6 and v7.

But sure, I'm open to ideas and experiments to see how things might actually work out. Anything that gives us more specific data on possibilities and limitations is good.

dai-shi commented 4 years ago

Issue 1: Existing React Redux apps that run in SyncMode should run in Concurrent Mode (createRoot)

How does Redux/React-Redux not support concurrent mode today? Is this just because of tearing?

This question is very important. There are two cases: a) existing apps break without the use of useTransition b) existing apps break with the use of useTransition (direct use like @eddyw does, or using somewhere in code not related to react-redux)

By using useLayoutEffect, we can de-opt to sync mode even if we are in concurrent mode, so we could do that when we find inconsistency from the external store.

Issue 2: Existing React Redux apps to get benefit of concurrent rendering (priority)

Has anyone looked into use-subscription hook? It was created at Facebook and is designed to safely manage subscriptions in concurrent mode.

I'm hoping if we use the same technique in use-subscription, we might be able to get benefit of priority. But because redux store is external anyway, I'm not sure how much benefit we could get.

Issue 3: What would Suspense-oriented React Redux apps look like and how a library should support new best practices

So, one in question is how we could live with <Suspense> and useTransition.

"lack of state branching"

Based on @eddyw 's codesandbox:

salvoravida commented 4 years ago

This is from my experience and understanding from testing CM:

  1. The tearing issue is described here by @markerikson -> https://stackoverflow.com/questions/54891675/what-is-tearing-in-the-context-of-the-react-redux This is what the mentioned PR is trying to solve. That should make existing RR apps just work in CM (consistent rendering). Let's call it "concurrent-mode safe"
  2. What doesn't work? I made this small demo: https://codesandbox.io/s/cm-simple-test-x5umg This doesn't use Redux or React-Redux. Test this in the following ways:
  • First test: click "Start Suspense", then click several times "Increment with Transition", then "Stop Suspense" You'll see that while you were incrementing, nothing was displayed on screen and this update happened on a "separate branch" of the state in memory until you stopped the transition.
  • Second test: click "Start Suspense", then click several times "Increment with Transition", then click "Increment without Transition" a couple of times, then click on "Stop Suspense" You'll see something confusing lol When you do "Increment with Transition" while in transition, this updates the forked state in memory, but "Increment without Transition" is updating both, the forked state and the current state. When you stop the transition, both merged

The issue is, Redux is not able to create a "branch" of the state and update it in memory. Every action results in the whole store being updated, regardless of using transition or not. Now, React-Redux does keep the slice of the state you subscribed to in React state, so technically, it's possible to dispatch an action wrapped within a transition, since this is sync, it'd result in a state update within a transition and React will know to update this forked state. However, this behaves differently if you update the same slice of state with & without transition while a component is suspended. This is the same demo but with Redux & React-Redux with @salvoravida changes: https://codesandbox.io/s/cm-simple-test-tx34i

Do the "second test" described above and compare what happens. So, it's not possible to "fix" this behavior because .. well, we can't. Redux just doesn't have a way to do branches (like git branches) of state since it isn't even aware of React.

i'm not SO SURE that React "branch merge" is a such desiderable thing. it works with counter and setCounter( prevCounter => prevCounter +1), but what about merging states that "time" depending on prev States? will the merge be consistent? i DO NOT THINK SO.

Probably React should NOT allow to equeue states inside startTransition, and outSide contemporalrly! Remember that when you MERGE you can have Conflict!

In other words React BEFORE allowing enqueue new States outside a startTransition, should Flush ALL pending states on the same comp!

I think you have just found a CM bug!!!!

While the Redux version is OK!

eddyw commented 4 years ago

@salvoravida I made this sandbox: https://codesandbox.io/s/cm-simple-test-bj47d It's mentioned that React batches updates in the suspended component (I don't recall where is that πŸ™ˆ ). However, "batching" doesn't mean .. "batching" lol

To understand better, please try it in sandbox and check console. What happens is something like this:

initialState => { counter: 0 }
// with transition
action1 = dispatch(...) => merge(initialState, action1) <<< counter: 1
action2 = dispatch(...) => merge(initialState, action1, action2) <<< counter: 2
action3 = dispatch(...) => merge(initialState, action1, action2, action3) <<< counter: 3
// without transition
action 4 = dispatch(...) => set initialState => { counter: 1 } <<< this is displayed
=> merge(
  initialState, <<< new is { counter: 1 }
  action1, <<< counter: 2
  action2, <<< counter: 3
  action3, <<< counter: 4 (this is what you'll see once you stop Suspense)
)

This is how it's batched, it's like every previous action is re-dispatched on top of the initial value. If value changes outside of transition, that becomes the new initial value and every other update is applied on top of it. This is my understanding of how it works based on testing.

This is not actually documented, it's just said to be ""batched"". However, testing it, it works like every new set value or dispatched action within a transition gets applied on top of previous dispatched actions. Technically, it replays all previous actions and adds the latest dispatched on top. So, like rebase.. literally haha

This means, it is actually possible to do this with Redux. The following would be the equivalent of the above useReducer. It'd mean, create a "temporary store" using the same store reducer/s with the initial value as current store value. Every dispatched action within a transition would go to "temporary store" (speudo-code):

temporaryStore = createStore(store.reducer, store.getState()) // state = { counter: 1 } 
... // within transition
dispatch({ type: 'INCREMENT' }) => // action 1
    temporaryStore = createStore(reducer, store.getState())
    temporaryStore.dispatch({ type: 'INCREMENT' }) => { counter: 2 }
dispatch({ type: 'INCREMENT' }) => // action 2
    temporaryStore = createStore(reducer, store.getState())
    temporaryStore.dispatch({ type: 'INCREMENT' }) => { counter: 2 }
    temporaryStore.dispatch({ type: 'INCREMENT' }) => { counter: 3 }
dispatch({ type: 'INCREMENT' }) => // action 3
    temporaryStore = createStore(reducer, store.getState())
    temporaryStore.dispatch({ type: 'INCREMENT' }) => { counter: 2 }
    temporaryStore.dispatch({ type: 'INCREMENT' }) => { counter: 3 }
    temporaryStore.dispatch({ type: 'INCREMENT' }) => { counter: 4 }
... // outside transition
dispatch({ type: 'INCREMENT' })
    store.dispatch({ type: 'INCREMENT' }) => { counter: 2 }
    temporaryStore = createStore(reducer, store.getState())
    temporaryStore.dispatch({ type: 'INCREMENT' }) => { counter: 3 }
    temporaryStore.dispatch({ type: 'INCREMENT' }) => { counter: 4 }
    temporaryStore.dispatch({ type: 'INCREMENT' }) => { counter: 5 }
... on Suspense Stop, applies batch of updates:
   store.dispatch({ type: 'INCREMENT' }) => { counter: 3 }
   store.dispatch({ type: 'INCREMENT' }) => { counter: 4 }
   store.dispatch({ type: 'INCREMENT' }) => { counter: 5 } <== it's in sync with temporaryStore

This is why it's said that it's "batched" and it works "like rebase" because every action is applied on top of the master branch but on a separate branch. At the end, you rebase your changes to the master branch (replay all work).

Where this wouldn't work? The fundamental difference between Redux and React useReducer is that, React's dispatch is pure while in Redux, this can be messed with if you apply middleware on top of it such as .. sagas, so you can't really dispatch the same actions in both stores without side-effects. For instance, if dispatch({ type: 'FETCH' }) starts a http request, then dispatch is not pure and you'll end up having the same request in a temporary store.

salvoravida commented 4 years ago

@eddyw i come on the same conclusion, react on start transition fork the states one with ALL setStates and one without transaction states (the temporary UI) on Stop transaction it will not MERGE but SWAP the branches....

my idea is to do an redux async, that support store Fork and Swap, even if with middlewares.

the main problem is that all of this, is not documented , and we are here testing , debugging, and guessing React goals... lol πŸŽ‰

eddyw commented 4 years ago

@salvoravida it doesn't SWAP the branches, it replays all the actions in the "master" branch. In the above demo, you can see that once you click on "Stop Suspense", it will log once again every single action that was dispatched while in transition, so it literally is like REBASE.

It's more or less like doing this:

const previousActions = []
const createTmpStore = () => createStore(reducer, store.getState(), c => (...args) => {
  const str = c(...args)
  const orgDispatch = str.dispatch
  const dispatch = action => {
     previousActions.push(action)
     return orgDispatch(action)
  }
  previousActions.forEach(action => orgDispatch(action)) // replay previous actions
  return { ...str, dispatch }
})

// start transition
let tmpStore = createTmpStore()
...
dispatch(...) => tmpStore = createTmpStore().dispatch(...)
dispatch(...) => tmpStore = createTmpStore().dispatch(...)
dispatch(...) => tmpStore = createTmpStore().dispatch(...)
previousActions.length => 3 [{ ... }, { ... }, { ... }]

// stop transition
previousActions.forEach(act => store.dispatch(act))
previousActions.length = 0
// <<<<< note, it doesn't swap, it "rebases" all previous actions on top of current store
eddyw commented 4 years ago

Hm, so, I did this: https://codesandbox.io/s/cm-redux-e6m2i It's a store enhancer for Redux. It's ugly but works. It creates the store but it doesn't allow dispatching actions outside of a React app. When used in a React app, useReducer handles internally the Redux reducer, so React becomes owner of the state.

This is just an idea, in the above, the enhancer over-writes getState, subscribe, and dispatch. So basically, the whole thing that makes Redux .. Redux to make React owner of the state. It will make no sense at first to use Redux at all .. but, the idea is that, taking out the enhancer, it should still work without React, in which case, using Redux still makes sense even if it's over-writing the whole logic behind it.

Maybe making an enhancer that returns a compatible Redux API actually makes sense. This just to make React owner of the state, so we don't need to worry about "fixing Redux" since React already has useReducer which does basically the same as createStore. Middleware is a complete different story to worry about.

salvoravida commented 4 years ago

i cannot agree at all to make react own redux state. redux state can be used outside of react, for others 1000 reasons. like microfronteds communication, orchestrator, iframe channel etc.. if we use Redux, we do not want to "depends" on react.

maybe we shoud create a redux with branches.

dai-shi commented 4 years ago

Issue 3: What would Suspense-oriented React Redux apps look like and how a library should support new best practices

@eddyw Wow, nice try. And it raises a question: What is Redux? My conclusion is state branching (which no longer is a single source) is not possible with the current Redux lib. So, I think there are two options: 1) keep backward compatibility and give up state branching (still apps work in CM and Suspense. useTransition to some extent). 2) break backward compatibility for state branching and create a new React Redux lib that will support say 80% of existing apps. Well, it might be too early for the conclusion. Let's keep discussing...

eddyw commented 4 years ago

i cannot agree at all to make react own redux state. redux state can be used outside of react, for others 1000 reasons. like microfronteds communication, orchestrator, iframe channel etc.. if we use Redux, we do not want to "depends" on react.

maybe we shoud create a redux with branches.

Just throwing random ideas. The above would work dispatching actions outside of React, only once React has initialized. Although, I can see many issues with my implementation (even if done right). For example, if you have two react apps, which is completely valid, the state here wouldn't be able to be shared since only one root instance will own the state.

Issue 3: What would Suspense-oriented React Redux apps look like and how a library should support new best practices

@eddyw Wow, nice try. And it raises a question: What is Redux? My conclusion is state branching (which no longer is a single source) is not possible with the current Redux lib. So, I think there are two options: 1) keep backward compatibility and give up state branching (still apps work in CM and Suspense. useTransition to some extent). 2) break backward compatibility for state branching and create a new React Redux lib that will support say 80% of existing apps. Well, it might be too early for the conclusion. Let's keep discussing...

I honestly don't see how it could break backwards compatibility. This "branching" only happens when you set state within transition. Current apps just don't use transition.

Now, if you move your app to CM mode, then start using transition and suspense with React Redux, that's migration and refactoring.

If you mean that current apps using RR, should work the same way in CM as they worked before even when using transition, then that's a wrong assumption. In CM, you can't avoid branching of state if you're using transition, I mean .. just don't randomly wrap everything is startTransition. RR uses React state internally to keep selectors, for instance, so if a dispatched action is within a transition, there will always be branching in the selectors that update because of that dispatched action. The issue is, the components that will see branching outside of Suspense will just bail out of re-rendering, if the component is force-rendered, then it'll display the latest value.

So, if we just fix tearing and keep everything else as is, this would be the "best practice" rule: – If you dispatch an action within a transition, make sure the slice of state that is changed only affects the components within Suspense. If you have a component subscribed to the same slice of state outside of Suspense, then expect it to display inconsistent data.

This is what "support 80% of CM features with RR" would mean: useTransition and Suspense works as long as you follow that rule.

However, in practice this could be difficult to follow in big React apps.

What I got so far is that RR could possibly do little to have full CM support. The last "20% missing support" is branching on Redux itself which probably shouldn't be RR responsibility? Since we don't have an "Async Redux", it's difficult to know if RR could help any further.

Idk, I'll just keep on experimenting, it's a nice learning experience lol

salvoravida commented 4 years ago

@eddyw https://twitter.com/dan_abramov/status/1193963224540090380

i think we cant yet understanding how CM branches merge/swap we have to wait for stable version