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.

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

Concurrent Mode #1351

Open
brunolemos opened this issue Jul 8, 2019 · 69 comments
Open

Concurrent Mode #1351

brunolemos opened this issue Jul 8, 2019 · 69 comments

Comments

@brunolemos
Copy link

@brunolemos brunolemos commented Jul 8, 2019

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
Copy link
Contributor

@markerikson markerikson commented Jul 8, 2019

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.

Loading

@dai-shi
Copy link
Contributor

@dai-shi dai-shi commented Jul 10, 2019

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.

Loading

@gcloeval
Copy link

@gcloeval gcloeval commented Sep 20, 2019

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!

Loading

@markerikson
Copy link
Contributor

@markerikson markerikson commented Sep 20, 2019

@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.

Loading

@Guria
Copy link

@Guria Guria commented Nov 7, 2019

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

Loading

@markerikson
Copy link
Contributor

@markerikson markerikson commented Nov 7, 2019

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.

Loading

@markerikson markerikson pinned this issue Nov 7, 2019
@salvoravida
Copy link

@salvoravida salvoravida commented Nov 7, 2019

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.

Loading

@markerikson
Copy link
Contributor

@markerikson markerikson commented Nov 7, 2019

@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.

Loading

@eddyw
Copy link

@eddyw eddyw commented Nov 7, 2019

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

  • Suspense just tells to display a fallback UI if some component is waiting for some data to be "ready"
  • useTransition (startTransition), tells Suspense to keep the old UI visible for a specific time (setup with { timeoutMs })

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.

Loading

@markerikson
Copy link
Contributor

@markerikson markerikson commented Nov 7, 2019

Related issue: JustSift/ReSift#32

Loading

@salvoravida
Copy link

@salvoravida salvoravida commented Nov 7, 2019

@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?

Loading

@markerikson
Copy link
Contributor

@markerikson markerikson commented Nov 7, 2019

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

Loading

@dai-shi
Copy link
Contributor

@dai-shi dai-shi commented Nov 7, 2019

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.

Loading

@salvoravida
Copy link

@salvoravida salvoravida commented Nov 7, 2019

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.

Loading

@dai-shi
Copy link
Contributor

@dai-shi dai-shi commented Nov 8, 2019

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.

Loading

@salvoravida
Copy link

@salvoravida salvoravida commented Nov 8, 2019

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.

Loading

@salvoravida
Copy link

@salvoravida salvoravida commented Nov 8, 2019

so if folks want IMMEDIATE update from everywhere then

function useImmediateDispatch() {
  const dispatch = useDispatch();
  return (...args) => {
    unstable_runWithPriority(unstable_UserBlockingPriority, () => {
      dispatch(...args);
    });
  };
}

Loading

@salvoravida
Copy link

@salvoravida salvoravida commented Nov 8, 2019

removed.

Loading

@eddyw
Copy link

@eddyw eddyw commented Nov 8, 2019

@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 🙈

Loading

@salvoravida
Copy link

@salvoravida salvoravida commented Nov 8, 2019

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 #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)

Loading

@salvoravida
Copy link

@salvoravida salvoravida commented Nov 8, 2019

@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.

Loading

@salvoravida
Copy link

@salvoravida salvoravida commented Nov 8, 2019

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

Regards!

Loading

@salvoravida
Copy link

@salvoravida salvoravida commented Nov 8, 2019

@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 !!

Loading

@salvoravida
Copy link

@salvoravida salvoravida commented Nov 8, 2019

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.

Loading

@dai-shi
Copy link
Contributor

@dai-shi dai-shi commented Nov 8, 2019

@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...

Loading

@eddyw
Copy link

@eddyw eddyw commented Nov 8, 2019

@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

Loading

@eddyw
Copy link

@eddyw eddyw commented Nov 11, 2019

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.

Loading

@salvoravida
Copy link

@salvoravida salvoravida commented Nov 11, 2019

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.

Loading

@dai-shi
Copy link
Contributor

@dai-shi dai-shi commented Nov 11, 2019

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...

Loading

@eddyw
Copy link

@eddyw eddyw commented Nov 11, 2019

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

Loading

@salvoravida
Copy link

@salvoravida salvoravida commented Nov 11, 2019

@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

Loading

@dai-shi
Copy link
Contributor

@dai-shi dai-shi commented Nov 11, 2019

@eddyw Thanks for the summary. Totally agreed.

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

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

If we don't use Suspense and useTransition, which is my Issue 1 & 2, there's no branching. There's little left to do hopefully, unless we find some other issues. (I think there are.)

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

If we useTransition, that requires some rules or limited behaviors. That's probably fine. What about middleware? You said it's a different story, but can we set a small set of rules to useTransition properly with middleware? I mean, it's OK to set rules for RR, but we would need to set rules for Redux too for branching. (So, I should have said backward compatibility with useTransition for the entire redux eco system, which might be a wrong assumption.)

But, that's still a tiny issue. Honestly, if I were to use Suspense and useTransition, I wouldn't put loading state in redux store. I wouldn't want to write throw promise in my app code. Some middleware may not work. It looks to me a dead end.

Loading

@salvoravida
Copy link

@salvoravida salvoravida commented Feb 8, 2020

facebook/react#18000

🎉 habemus papam ! 🎉

Loading

@markerikson
Copy link
Contributor

@markerikson markerikson commented Feb 14, 2020

Brian Vaughn just posted the RFC for the useMutableSource hook:

reactjs/rfcs#147

Loading

@dai-shi
Copy link
Contributor

@dai-shi dai-shi commented Feb 21, 2020

While it's quiet here, here's my implementation of react-redux basic hooks with the currently proposed useMutableSource.

 const StoreContext = createContext(); 
 const subscribe = (store, callback) => store.subscribe(callback); 
 const Provider = ({ store, children }) => { 
   const contextValue = useMemo(() => ({ 
     source: createMutableSource(store, () => store.getState()), 
     dispatch: store.dispatch, 
   }), [store]); 
   return ( 
     <StoreContext.Provider value={contextValue}> 
       {children} 
     </StoreContext.Provider> 
   ); 
 }; 
 const useDispatch = () => useContext(StoreContext).dispatch; 
 const useSelector = (selector) => { 
   const selectorRef = useRef(selector); 
   useLayoutEffect(() => { 
     selectorRef.current = selector; 
   }); 
   const getSnapshot = useCallback((store) => selectorRef.current(store.getState()), []); 
   const { source } = useContext(StoreContext); 
   return useMutableSource(source, getSnapshot, subscribe); 
 }; 

I'm experimenting it with my tool.

Loading

@markerikson
Copy link
Contributor

@markerikson markerikson commented Feb 21, 2020

@dai-shi : yeah, I think my immediate questions would be:

  • does this improve the pass/fail results on the CM test?
  • Does this affect the perf benchmarks in some way?
  • Does this relate to Suspense handling at all?

Loading

@dai-shi
Copy link
Contributor

@dai-shi dai-shi commented Feb 22, 2020

does this improve the pass/fail results on the CM test?

According to my observation, it improves but not completely like described in the gist.
To me, it still seems to behave like useSubscription. Not sure where the root cause is. Some of my implementation or assumption could be wrong.

Does this affect the perf benchmarks in some way?

I've done a preliminary test with use-context-selector: dai-shi/use-context-selector#12 (comment)
So far, so good. This is just a simple benchmark with just one or two useSelector. We would eventually need to test with react-redux-benchmarks.

Does this relate to Suspense handling at all?

Depends on what you mean by Suspense handling. State branching with useTransition would never be possible with an external store like Redux. Other than that, it should already be working as is.
So, the answer is no, I don't think this relates to it.

Loading

@dai-shi
Copy link
Contributor

@dai-shi dai-shi commented Feb 22, 2020

State branching with useTransition would never be possible with an external store like Redux.

I was wrong. Oh, I was right.

Loading

@JoseExposito
Copy link

@JoseExposito JoseExposito commented Mar 11, 2020

useMutableSourceHook just got merged into master:
facebook/react#18000 (comment)

Loading

@markerikson
Copy link
Contributor

@markerikson markerikson commented Mar 11, 2020

I don't have time to play with this myself atm, but I'd be very interested to see a PR that tries to convert useSelector to be based on useMutableSource. I'm also curious if connect can be made to work with it as well.

Brian apparently put together an over-simplified example of how we might use it:

https://codesandbox.io/s/react-redux-usemutablesource-eyxoe

Loading

@dai-shi
Copy link
Contributor

@dai-shi dai-shi commented Mar 12, 2020

reactjs/rfcs#147 (comment)
FWIW, I tried the stale props spec in #1505 with use-context-selector v2 with useMutableSource, and it failed unfortunately.

Loading

@nickmccurdy
Copy link
Contributor

@nickmccurdy nickmccurdy commented Mar 12, 2020

I've been making some progress with useMutableSource, but I need to figure out why act() isn't waiting for dispatches in tests.

Loading

@dai-shi
Copy link
Contributor

@dai-shi dai-shi commented Mar 12, 2020

@nickmccurdy Just a guess, but please try rtl.act() instead of act().

That worked for me: https://github.com/reduxjs/react-redux/pull/1536/files#diff-8a4f74cb696e74165240c9af9cb0ce45R188

Loading

@dai-shi
Copy link
Contributor

@dai-shi dai-shi commented Mar 12, 2020

So, I re-implemented my reactive-react-redux with useMutableSource.
https://www.npmjs.com/package/reactive-react-redux/v/5.0.0-alpha.1
dai-shi/reactive-react-redux#48
This version of the API is slightly different from react-redux v7. It doesn't depend on Context. Developers will create a context at their requirement. (BTW, I heard MobX takes the same approach.)
Of course, it can be used without Context if one wants.


In terms of implementation for react-redux v7, looking at my snippet again, I think this one still suffers from the race condition issue #1536.

Loading

@nickmccurdy
Copy link
Contributor

@nickmccurdy nickmccurdy commented Mar 13, 2020

Nice. Does this still support multiple store contexts like React Redux?

Loading

@dai-shi
Copy link
Contributor

@dai-shi dai-shi commented Mar 14, 2020

Yes, I mean r-r-r doesn't care how developers use contexts with it.
Here's the example how to use a context with the API in r-r-r.

https://github.com/dai-shi/reactive-react-redux/blob/a57a033f676130283a414990d8cc4d8486a608fb/examples/12_async/src/context.ts

Loading

@wurstbonbon
Copy link
Contributor

@wurstbonbon wurstbonbon commented Mar 15, 2020

A simplistic implementation of connect using useMutableSource might look something like this:

const requiresOwnProps = mapStateOrMapDispatch => {
  return (
    typeof mapStateOrMapDispatch === 'function' &&
    mapStateOrMapDispatch.length !== 1
  )
}

const applyMapStateToProps = (mapStateToProps, state, props) => {
  if (typeof mapStateToProps === 'function') {
    return requiresOwnProps(mapStateToProps)
      ? mapStateToProps(state, props)
      : mapStateToProps(state)
  }
  return EMPTY_OBJECT
}

const applyMapDispatchToProps = (mapDispatchToProps, dispatch, props) => {
  if (typeof mapDispatchToProps === 'function') {
    return requiresOwnProps(mapDispatchToProps)
      ? mapDispatchToProps(dispatch, props)
      : mapDispatchToProps(dispatch)
  }
  if (mapDispatchToProps && typeof mapDispatchToProps === 'object') {
    return bindActionCreators(mapDispatchToProps, dispatch)
  }
  return { dispatch }
}

const defaultMergeProps = (stateProps, dispatchProps, ownProps) => {
  return { ...ownProps, ...stateProps, ...dispatchProps }
}

const useStableObject = obj => {
  const objRef = useRef(obj)
  useLayoutEffect(() => {
    objRef.current = obj
  }, [obj])
  return shallowEqual(obj, objRef.current) ? objRef.current : obj
}

const useStateProps = (mapStateToProps, ownProps) => {
  const { source } = useContext(ReactReduxContext)
  const propsForMapState = requiresOwnProps(mapStateToProps) ? ownProps : null

  const getSnapshot = useCallback(
    store =>
      applyMapStateToProps(mapStateToProps, store.getState(), propsForMapState),
    [mapStateToProps, propsForMapState]
  )
  const stateProps = useMutableSource(source, getSnapshot, subscribe)
  return useStableObject(stateProps)
}

const useDispatchProps = (mapDispatchToProps, ownProps) => {
  const { dispatch } = useContext(ReactReduxContext)
  const propsForMapDispatch = requiresOwnProps(mapDispatchToProps)
    ? ownProps
    : null
  return useMemo(
    () =>
      applyMapDispatchToProps(
        mapDispatchToProps,
        dispatch,
        propsForMapDispatch
      ),
    [mapDispatchToProps, dispatch, propsForMapDispatch]
  )
}

const useMergeProps = (mergeProps, stateProps, dispatchProps, ownProps) => {
  return useMemo(() => mergeProps(stateProps, dispatchProps, ownProps), [
    mergeProps,
    stateProps,
    dispatchProps,
    ownProps
  ])
}

const connect = (
  mapStateToProps = null,
  mapDispatchToProps = null,
  mergeProps = defaultMergeProps
) => WrappedComponent => {
  const MemoizedWrappedComponent = React.memo(WrappedComponent)

  if (mapStateToProps) {
    return React.memo(props => {
      const ownProps = useStableObject(props)
      const stateProps = useStateProps(mapStateToProps, ownProps)
      const dispatchProps = useDispatchProps(mapDispatchToProps, ownProps)
      const mergedProps = useMergeProps(
        mergeProps,
        stateProps,
        dispatchProps,
        ownProps
      )
      return <MemoizedWrappedComponent {...mergedProps} />
    })
  } else {
    return React.memo(props => {
      const ownProps = useStableObject(props)
      const dispatchProps = useDispatchProps(mapDispatchToProps, ownProps)
      const mergedProps = useMergeProps(
        mergeProps,
        EMPTY_OBJECT,
        dispatchProps,
        ownProps
      )
      return <MemoizedWrappedComponent {...mergedProps} />
    })
  }
}

Loading

@markerikson
Copy link
Contributor

@markerikson markerikson commented Jun 10, 2021

The React team has announced plans for React 18, per https://reactjs.org/blog/2021/06/08/the-plan-for-react-18.html , and created a "React 18 Working Group" discussion forum at https://github.com/reactwg/react-18/discussions .

I just filed an issue asking for people to try out React-Redux v7 and the React 18 alphas together, and let us know what sorts of issues pop up. We'd love to have folks help out with this!

See #1732 - Investigation: try React-Redux v7 with React 18 alphas for details.

Loading

@dai-shi
Copy link
Contributor

@dai-shi dai-shi commented Jun 10, 2021

Sharing my experience related with uMS in React 18 alpha, which is unstable_, one thing to note is "stable selectors".

I noticed while experimenting "tearing" with redux with uMS, wrapping a selector with useCallback is necessary.

https://github.com/dai-shi/will-this-react-global-state-work-in-concurrent-rendering/blob/d8dee99d068b980b28aacddc476cb0643ccb63d9/src/redux-use-mutable-source/index.js#L38
changing this to

  const getSnapshot = (s) => selectCount(s.getState());

causes infinite loop like behavior.

In reactive-react-redux, I added a note "selector has to be stable. Either define it outside render or use useCallback if selector uses props."

In zustand, I added a note "It is generally recommended to memoize selectors with useCallback.", but this is going to be a requirement with breaking change.

Loading

@markerikson
Copy link
Contributor

@markerikson markerikson commented Jun 10, 2021

Hmm. That could be... annoying :) It's so very common to just do const todos = useSelect(state => state.todos), and inline the selector declaration.

Loading

@markerikson markerikson unpinned this issue Jun 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.