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

add re-rxjs #18

Merged
merged 1 commit into from
Jun 16, 2020
Merged

add re-rxjs #18

merged 1 commit into from
Jun 16, 2020

Conversation

josepot
Copy link
Contributor

@josepot josepot commented Jun 11, 2020

Hi @dai-shi

Thanks a ton for this project!

I've been working on these react-rxjs bindings. This thing started as an experiment for my side-projects and we ended up using it in my company. After some iterations I think that now they are mature enough to be published on their own library, document them properly, etc. I still have a long way to go, though.

I've recently made some API changes to fully support React concurrent's mode. After I made the changes I thought that I would give a shot to these tests and I'm pretty happy with the results 🙂. Could you please add re-rxjs into the main list? I'm currently working on the docs and I would like to be able to include a link to these results.

Thanks

@dai-shi
Copy link
Owner

dai-shi commented Jun 12, 2020

Hi, thanks for your support.

Looking at the code at https://github.com/re-rxjs/re-rxjs/blob/master/src/useObservable.ts,
I don't understand why it passes all the tests.
Unless we use useMutableSource or use only one React state, that wouldn't happen. Either, 1) I misunderstand your code, 2) your spec is wrong, or 3) the tool is wrong.

If you have any explanation how to support CM, I'm happy to hear it.


const [useCounter] = connectObservable(
merge(normalClicks$, externalClicksAsap$).pipe(
scan((x) => x + 0.5, 0),
Copy link
Owner

Choose a reason for hiding this comment

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

Uh, nice.

<button
type="button"
id="normalIncrement"
onClick={isPending ? transitionIncrement : normalIncrement}
Copy link
Owner

Choose a reason for hiding this comment

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

This is weird. Just do this:

Suggested change
onClick={isPending ? transitionIncrement : normalIncrement}
onClick={normalIncrement}

const normalIncrement = () => normalClicks$.next();

const externalClicks$ = new Subject();
// This is only needed to prevent the tearing with auto increment (check #9)
Copy link
Owner

Choose a reason for hiding this comment

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

check 9 is to distinguish useSubscription and useMutableSource, so you wouldn't need to treat it specially. I don't know how asapScheduler helps here, but the purpose of the check is not to bypass like that. (I mean, that means the check 9 is not working as I expect.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both useSubscription and useMutableSource are custom hooks built on top of the existing primitive hooks, meaning that they don't do anything that can't be done from user-land.

I've learned a lot from understanding what they do, and in early versions I was using useSubscription. However, once I better understood what the hook was doing and how RxJS subscriptions behave, I decided to build my own integration for RxJS subscriptions.

The asapScheduler doesn't try to "bypass" a test. That's not at all what it is for. I will document this, but the thing is that when working with the RxJS bindings, if you are subscribed to a data-source that comes from outside of React, then you should be observing it with the asapScheduler because otherwise react won't be able to batch the updates.

So, no, I'm not trying to cheat here 🙂 this is how the bindings should be used with RxJS for inputs that don't come from withing React event-handlers.

Copy link
Owner

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

Most importantly, I don't get why your lib passes check 2. Do you have any idea?

@josepot
Copy link
Contributor Author

josepot commented Jun 12, 2020

Looking at the code at https://github.com/re-rxjs/re-rxjs/blob/master/src/useObservable.ts,
I don't understand why it passes all the tests.
Unless we use useMutableSource or use only one React state, that wouldn't happen.

I don't agree with this statement. useMutableSource does make sure that a generic mutable source behaves correctly with react, but why can't I leverage the way that RxJS behaves to accomplish the same thing? That's in a nutshell what this library is about and the reason why I think it's so cool 🙂

Either, 1) I misunderstand your code, 2) your spec is wrong, or 3) the tool is wrong.

I think that you misunderstood my code. I still have to cleanup the code and document a lot of things, and I'm planing on writing an advanced blog-post explaining the magic behind it.

If you have any explanation how to support CM, I'm happy to hear it.

Sorry, but I didn't understand this last part... What do you mean by "how to support CM"?

Actually, sorry, scratch that 😅 ! CM = Concurrent Mode, right... Yes, actually I have some suggestions for improving these tests. At least I have a suggestion for improving the "proper branching with transition" test... It's related to the comment that I haven't answered yet. I don't have time right now, but I will try to answer it as soon as I can.

Thanks for getting back at me so quickly!

@dai-shi
Copy link
Owner

dai-shi commented Jun 12, 2020

That's in a nutshell what this library is about and the reason why I think it's so cool 🙂

Yeah, but my point is my tool does not 100% behave how I expect. So, this is a good chance to improve the tool itself.
I did try to reproduce what I tested by hand with jest spec, but it's tricky and sometimes not stable.

Both useSubscription and useMutableSource are custom hooks built on top of the existing primitive hooks

Yes for useSubscription and no for uMS, AFAIU.

Most of my libs are based on useSubscription technique now. (not directly using it)
If you find another way, I'm sooooooo much interested in it.

Look forward to hearing more from you.

"proper branching with transition"

I'm not sure if you could understand what I'd like to test from the jest spec.
Let me try to explain it.
We have "normal button", "transition button", and "count".

A: At first, count=0
B: if we click transition button twice, count will be one,
C: click the button twice again within the transition, count will become two after one.
D: but while it's in transition, we click normal button twice, this will not wait the transition, so it will increment the counter, before C is effective, (I mistook as opposite) this will be in a separate branch and only merged after C is done. this is the whole point of the test.

In the result we will see 0->1->2 while pending, and finally C is done and see 3 and pending finished.

@dai-shi
Copy link
Owner

dai-shi commented Jun 14, 2020

@josepot Just run manual tests with your branch. This is truly amazing.
I still don't understand how this is happening underneath.

https://github.com/re-rxjs/re-rxjs/blob/704245ade5c64de9d0155c57934a6cd95faa11ea/src/useObservable.ts#L59-L62
Does this the trick? Really look forward to your explanation.
Can you explain why this technique can't be applied to react-redux? or can it?

Modified your code to be inline with others. Here's the diff:

diff --git a/src/re-rxjs/index.js b/src/re-rxjs/index.js
index c0c3372..970e9fb 100644
--- a/src/re-rxjs/index.js
+++ b/src/re-rxjs/index.js
@@ -1,12 +1,15 @@
 import React, { unstable_useTransition as useTransition } from 'react';
-import { Subject, merge, asapScheduler } from 'rxjs';
+import { Subject, asapScheduler } from 'rxjs';
 import {
   map, scan, startWith, observeOn,
 } from 'rxjs/operators';
 import { connectObservable } from 're-rxjs';
+
 import {
   syncBlock,
   useRegisterIncrementDispatcher,
+  initialState,
+  reducer,
   ids,
   useCheckTearing,
 } from '../common';
@@ -14,16 +17,12 @@ import {
 const normalClicks$ = new Subject();
 const normalIncrement = () => normalClicks$.next();
 
-const externalClicks$ = new Subject();
-// This is only needed to prevent the tearing with auto increment (check #9)
-const externalClicksAsap$ = externalClicks$.pipe(observeOn(asapScheduler));
-const externalIncrement = () => externalClicks$.next();
-
 const [useCounter] = connectObservable(
-  merge(normalClicks$, externalClicksAsap$).pipe(
-    scan((x) => x + 0.5, 0),
-    map(Math.floor),
-    startWith(0),
+  normalClicks$.pipe(
+    observeOn(asapScheduler),
+    startWith(initialState),
+    scan((x) => reducer(x, { type: 'increment' })),
+    map((x) => x.count),
   ),
 );
 
@@ -36,7 +35,7 @@ const Counter = React.memo(() => {
 const Main = () => {
   const count = useCounter();
   useCheckTearing();
-  useRegisterIncrementDispatcher(externalIncrement);
+  useRegisterIncrementDispatcher(normalIncrement);
   const [localCount, localIncrement] = React.useReducer((c) => c + 1, 0);
   const [startTransition, isPending] = useTransition();
   const transitionIncrement = () => {
@@ -50,7 +49,7 @@ const Main = () => {
       <button
         type="button"
         id="normalIncrement"
-        onClick={isPending ? transitionIncrement : normalIncrement}
+        onClick={normalIncrement}
       >
         Increment shared count normally (two clicks to increment one)
       </button>

Could you review it?

It may be unsatisfying that check 7 doesn't pass. But, the purpose of the check is to detect state branching and state branching is technically impossible with external state. (I didn't realize isPending ? : bypasses it. I wonder if there's a way to detect it.)

I still don't understand the asapScheduler and how your lib solves the tearing, however, I'm happy to merge with the diff above even without me understanding your lib at the moment.

BTW, is there a better way to dispatch { type: 'increment' } in the event source, instead of directly feeding it into scan? Please note that I will be modifying reducer in src/common in the future, and I'd like to avoid depending on the implementation detail. (Would be good to fix mobx one.)

@josepot
Copy link
Contributor Author

josepot commented Jun 14, 2020

Hi @dai-shi !

First of, thanks a lot for your interest and for helping me at adding Re-RxJS into the list.

I promise you that I'm dying to explain in detail all what's happening behind the scenes. However, right now I'm smashed with work and I still have to:

  • Add a ton of tests to Re-RxJS plus improving the present ones
  • Finish the first set of documentation for the library
  • Add a good set of examples and use-cases
  • Finish writing a presentation blog-post for the library

I'm very sorry, but as I'm sure that you will understand I'm beyond busy with all those chores right now, but I promise that as soon as I have a chance I will explain everything in detail.

All that being said, I wanted to challenge one of your comments... Perhaps I misunderstood what you are trying to say, but I think that this statement is not accurate:

It may be unsatisfying that check 7 doesn't pass. But, the purpose of the check is to detect state branching and state branching is technically impossible with external state. (I didn't realize isPending ? : bypasses it. I wonder if there's a way to detect it.)

As you can see in this example Re-RxJS does handle branching perfectly well during transitions (try starting a transition and while the transition is happening, then play with the counter... When the transition ends, the counter has the correct state).

I will try to take some time in the next few days to explain why IMO the test "proper branching with transition" has some room for improvement... I could be wrong, but nevertheless I think that it's a conversation worth having.

Again, I'm sorry that I can't yet get back to you right now... Although, if you want to contribute to Re-RxJS, I would be super happy to get some help 😅

@voliva
Copy link

voliva commented Jun 14, 2020

I've also tried manual testing to check out how does re-rxjs manage to avoid tearing (test num. 2), and adding some logs to see the sequence I think it's due to re-rxjs using react's state to get and set the value.

In the case of react-state, the sequence of actions I see in the logs are:

  1. State gets incremented to 1
  2. It starts rendering each of the Count, all of these components receive value = 1
  3. State gets incremented to 2
  4. The remaining Count components continue rendering, receiving value = 1
  5. All Count components re-render, receiving value = 2

In libraries with tearing issues what happens is that in step 4, the remaining Count components receive value = 2 - which then it's the inconsistent state rendering known as tearing. That happens because that library is getting that value from an external source, and react can't know whether that value is from the current render loop or the next one.

What I think that re-rxjs does to work around this is that every component holds a state value through useReducer, and when there's an update in the stream, all the components subscribed to that stream will update their state (and so it will trigger a re-render for that component).
This way, if the stream emits a value while react is delaying the rendering of some components, those components will use a react primitive to dispatch that the state has changed, and react will make that change effective in the next render loop.

@dai-shi
Copy link
Owner

dai-shi commented Jun 14, 2020

@josepot Suppose you implement a transition with rxjs, it has to be in sync with React transition in your library code (I'm not sure even if it's possible) to get benefit from CM. If it's really implemented so, you don't need isPending ? : . You can assume it as a definition here of "proper branching".

@dai-shi
Copy link
Owner

dai-shi commented Jun 15, 2020

@voliva

This way, if the stream emits a value while react is delaying the rendering of some components, those components will use a react primitive to dispatch that the state has changed, and react will make that change effective in the next render loop.

Yes, this is important. What I'm not sure is it could still tear after the dispatch. (But, it won't)
So, if this is a real thing, why can't we apply the same technique to react-redux?
(I'm pretty sure I'm missing something.)

@dai-shi
Copy link
Owner

dai-shi commented Jun 15, 2020

Oh, I think I get it. It somehow synchronizes all renders.
Then my expectation (by the design definition of this tool) is check 3 should fail.

(No, check 3 passes fine.

@dai-shi
Copy link
Owner

dai-shi commented Jun 15, 2020

    re-rxjs [
      189, 183, 183, 186,
      193, 200, 171, 175,
      195, 193
    ]

(It's fast enough. Really amazing.

@resolritter
Copy link

resolritter commented Jun 16, 2020

This is my current understanding, but I'm by no means an expert in React's implementation, so here's a foreword of caution.

What I'm not sure is it could still tear after the dispatch

Here's a section which is relevant to the internal implementation details of useReducer

https://overreacted.io/a-complete-guide-to-useeffect/#why-usereducer-is-the-cheat-mode-of-hooks

The most relevant bit is:

The answer is that when you dispatch, React just remembers the action — but it will call your reducer during the next render

In the React source, this is seen in

https://github.com/facebook/react/blob/30b47103d4354d9187dc0f1fb804855a5208ca9f/packages/react-reconciler/src/ReactFiberHooks.new.js#L788

where markWorkInProgressReceivedUpdate is only called inside of the reducer-related functions.

Why it doesn't tear

Tearing happens when uncommitted values are derived from during render.

facebook/react#14099 (comment)

In concurrent mode, last render doesn't necessarily mean "latest committed state". So a low-priority render with new props or state would overwrite a reference used by current event handler.

A reducer's deterministic nature of processing everything in order of dispatch, and thus leading to the same result if it were replayed in the same order, is what enables their evaluation to be deferred, rather than eagerly evaluated during render passes.

Writing ref.current = value means: assign this value right now, because I could be using it in the next line. On the other hand, dispatch(action) means: the resulting value will be updated at some point in the future through this payload, i.e. the laziness is a given for how reducers work; the reducing process through the action queue is what allows it to be batchable, pausable and resumable.

This leads to the assumption: the reducer's function and value only is propagated in the commit phase of the component, which would be why that closure reducer from Dan Abramov's blog post can safely capture the parent's props.

Why the example works

Because the subscription's pushes are being enqueued as actions, thus only propagated during the commit phase. Through useReducer, React will resolve their update at the proper time, in which, by that point, each component will be able to access their own latest immutable copy of the state pushed from the Observable.


Those assumptions rely on the "Reducer value only propagates during the commit phase" idea. If that's correct, then it'd also mean only useLayoutEffect is the only safe place to derive state from props and mutable references, because that's when React will have settled down with all the render passes which might lead to inconsistencies, such as tearing.

So, if this is a real thing, why can't we apply the same technique to react-redux?

I think a similar implementation can be applied to react-redux, but it'd mean having those immutable store copies living in a reducer for each component, rather than a storage outside of React. I think that'd also make the connect APIs not work anymore, as props are subject to tearing problems.

@dai-shi
Copy link
Owner

dai-shi commented Jun 16, 2020

@resolritter Thanks for the note!
Oh, I forgot about the cheat mode, and I didn't realize when I saw useObservable code at a glance.
I think/thought I'm fairly familiar with the cheat mode, as I actually did it in react-redux,
one for stale props issue and another for concurrent mode.
reduxjs/react-redux#1505
reduxjs/react-redux#1509

Now, observing the behavior of re-rxjs, I'm not sure if I fully understand the cheat mode.
There might be a better implementation than what I did before. The implementation of re-rxjs looks so clean, and I'm sure I can learn something from it.

Anyway, my biggest concern about my misunderstanding is gone, even though I don't yet fully understand the implementation, I will merge this PR with some modifications I made here and in master.

We can continue the discussion in this PR even if it's closed, or one can open a new issue.

@dai-shi dai-shi merged commit de0fc18 into dai-shi:master Jun 16, 2020
@dai-shi
Copy link
Owner

dai-shi commented Jun 16, 2020

(wow, check 7 passes with my new check. Really impressive. I need to learn more about this.

@dai-shi
Copy link
Owner

dai-shi commented Jun 17, 2020

(btw, I still wonder how check 9 passes here. It does pass for sure and the tool seems OK. My gut feeling is there's some limitation somewhere. Hope to learn more eventually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants