-
Notifications
You must be signed in to change notification settings - Fork 46
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
add re-rxjs #18
Conversation
Hi, thanks for your support. Looking at the code at https://github.com/re-rxjs/re-rxjs/blob/master/src/useObservable.ts, 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), |
There was a problem hiding this comment.
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} |
There was a problem hiding this comment.
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:
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) |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this 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?
I don't agree with this statement.
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.
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! |
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.
Yes for useSubscription and no for uMS, AFAIU. Most of my libs are based on useSubscription technique now. (not directly using it) Look forward to hearing more from you.
I'm not sure if you could understand what I'd like to test from the jest spec. A: At first, count=0 In the result we will see 0->1->2 while pending, and finally C is done and see 3 and pending finished. |
@josepot Just run manual tests with your branch. This is truly amazing. https://github.com/re-rxjs/re-rxjs/blob/704245ade5c64de9d0155c57934a6cd95faa11ea/src/useObservable.ts#L59-L62 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 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 |
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:
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:
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 😅 |
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:
In libraries with tearing issues what happens is that in step 4, the remaining What I think that re-rxjs does to work around this is that every component holds a state value through |
@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 |
Yes, this is important. What I'm not sure is it could still tear after the dispatch. (But, it won't) |
|
(It's fast enough. Really amazing. |
This is my current understanding, but I'm by no means an expert in React's implementation, so here's a foreword of caution.
Here's a section which is relevant to the internal implementation details of https://overreacted.io/a-complete-guide-to-useeffect/#why-usereducer-is-the-cheat-mode-of-hooks The most relevant bit is:
In the React source, this is seen in where Why it doesn't tearTearing happens when uncommitted values are derived from during render. facebook/react#14099 (comment)
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 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 worksBecause the subscription's pushes are being enqueued as actions, thus only propagated during the commit phase. Through Those assumptions rely on the "Reducer value only propagates during the commit phase" idea. If that's correct, then it'd also mean only
I think a similar implementation can be applied to |
@resolritter Thanks for the note! Now, observing the behavior of re-rxjs, I'm not sure if I fully understand the cheat mode. 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. |
(wow, check 7 passes with my new check. Really impressive. I need to learn more about this. |
(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. |
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