Idiomatic rxjs #1

sourishkrout · 2020-08-18T05:46:05Z

Refactorage

grampelberg

Bunch of questions! I love that the stream got folded from 3 into one. I think my biggest questions are around how the caching works and how to get loading to toggle (but I suspect I know how to do that with a nested subscribe ...)

grampelberg · 2020-08-18T17:00:04Z

hooks/useFetch.js

-    tap(data),
+  const refresher = timer(0, 1000).pipe(
+    switchMap(() => getURL(url)),
+    publish(),


Okay, so this sets the timer which fires immediately and every second. It doesn't actually do anything though until something subscribes because of the publish? Why is the publish required if observables don't go hot until the subscription normally?

The publish is required to turn the timer into something multiple subscribers can subscribe to. Think about it like a multiplexer. The refresher stream which is subscribed to the timer stream won't start firing until it has at least 1 subscriber which has also called connect on the subscription to turn it hot. To avoid requiring subscribers having to call connect/disconnect is why the next line leverages the refCounter.

grampelberg · 2020-08-18T17:06:03Z

hooks/useFetch.js

+  const refresher = timer(0, 1000).pipe(
+    switchMap(() => getURL(url)),
+    publish(),
+    refCount(),


Why do publish/refCount instead of just share (or just using a subject)? From the docs:

Note that using the share operator is exactly the same as using the publish operator (making the observable hot) and the refCount operator in a sequence.

Yes, you could use share. However, depending on the use case you might want to use publishBehavior or publishReplay to get different characteristics some of which have their own shareReplay etc. Just got used to using them in conjunction since it's easier to play around with them individually. - Once you've got the right behavior you could condense a sequence of operator with combinator operatoers.

grampelberg · 2020-08-18T17:07:40Z

hooks/useFetch.js

-      data.subscribe((ev) => {
-        setData(ev.value.body)
-        setError(ev.error)
+    const fetcher = fetchData(url).pipe(


Will this toggle loading on refresh? I like the pattern here more, but loading will always just be false after the first time right?

Yes. It won't toggle it, it will work like an argument to instantiate a consumable stream to either be one or the other. Did you want streams to allow toggling refresh? Can probably make that happen.

Oh shit... i think I misunderstood and it just became clear to me. You're right I forgot to toggle loading when a refresh is attempted. Let me fix that.

grampelberg · 2020-08-18T17:09:59Z

hooks/useFetch.js

-      sub.add(startRefresh.subscribe())
+    let refresher = fetcher
+    if (!refresh) {
+      refresher = fetcher.pipe(take(1))


So, instead of taking the whole stream, this'll take a single one and then stop listening again? How does it fit in with the cache? Is the original stream kept in memory and the first item is taken? Or is it the latest item? Or am I not understanding at all? =)

take(1) will emit a complete to the subscriber signaling that it's complete. If you don't just want avoid duplication of fetcher using a cache you should be able to use publishBehavior instead of publish to relay the last value emitted to subsequent subscribers. Let me add that.

It sounds like this take(1) makes it so that if someone else is refreshing, no one else gets updates. That seems weird? I assumed that it would update all subscribers and not just the one taking refreshes.

@grampelberg do you mean when you have multiple useFetchMany consumers? E.g. below. - That works fine for me. The take(1) happens prior to publishBehavior(null), refCount(), which should make it so that you multiple subscribers can consume to emissions from the same source stream.

const Home = () => ( <> <Many /> <Many /> <Many /> <One /> </> )

grampelberg · 2020-08-18T17:11:13Z

hooks/useFetch.js

    }

+    const sub = refresher.subscribe((ev) => {
+      if (ev.error) {


Is there a reason you're guarding the error here? My thinking was that if ev.error is undefined, on the next refresh it'll get unset (imagine retries).

Notice the return in the subsequent line return setError(ev.error) which will avoid a null pointer exception for ev.value.body when an error occurs because ev.value will be null. Maybe not the most self-explanatory way to avoid this execption.

grampelberg · 2020-08-18T17:16:40Z

hooks/useFetch.js

-        filter((i) => !isError(i)),
-        dematerialize(),
-        updateBy('url'),
+        combineAll(),


Does this end up waiting for all to complete? The main reason I did this implementation was to allow results to stream back (especially when there's partial errors). I'd like to get results from some of the endpoints even when there's issues across them all (imagine a timeout holding everything up for 10 seconds).

Yes, it will lump the individual streams together and emit them as one value. You're right, this will result in the joint completion taking as long as the slowest one to complete. Let me improve on that.

I generally wouldn't recommend lumping unrelated requests together unless the consumer(s) requires it. I mean, I suppose useFetchMany is more of a convenience API.

grampelberg · 2020-08-18T17:17:22Z

hooks/useFetch.js

+          )
+          setError(errs)
+          setLoading(false)
+        }),
      )
      .subscribe(setData)


Won't setData end up adding Notification including errors and completes?

🤷‍♂️ , i have to admit that I refactored the rxjs plumping without fully getting into what the playloads look like or reasoning about it. My 1-2h best quick effort. - Should be easy to modify this to your expectations here.

grampelberg

My biggest question is around the refresh updates. I think I understand why it is an issue but I'd love some confirmation =)

grampelberg · 2020-08-20T19:45:03Z

hooks/useFetch.js

-      sub.add(startRefresh.subscribe())
+    let refresher = fetcher
+    if (!refresh) {
+      refresher = fetcher.pipe(take(1))


It sounds like this take(1) makes it so that if someone else is refreshing, no one else gets updates. That seems weird? I assumed that it would update all subscribers and not just the one taking refreshes.

grampelberg · 2020-08-20T20:15:51Z

hooks/useFetch.js

+      setError([])
+    })
+
+    of(setLoading(true))


This is definitely setting loading: false a bunch, but it doesn't appear to ever set it to true. I tried adding a couple types events and a concat, but that broke everything and I've not figured out why yet.

const refresher = timer(0, 1000).pipe( switchMap(() => concat( of({ kind: 'loading', state: true }), getURL(url).pipe( catchError((err) => of(err)), publishBehavior(null), refCount(), filter((ev) => ev !== null), map((resp) => ({ kind: 'response', ...resp })), ), of({ kind: 'loading', state: false }), ), ), )

Dang. I didn't properly test this with refresh=true. However, since of(...) is the source observable it will only ever run once, then complete. Without having tested it thoroughly I believe your revised refresher impl suffers from the same problem. Lemme see if I can fix this.

@grampelberg pushed some more commits; essentially split the refresher into two separate streams 1.) pulse (interval fires) & 2. fetcher (emits values from remote API). This will make it work.

grampelberg · 2020-08-20T20:23:22Z

hooks/useFetch.js

+    const valsub = vals.subscribe(setData)
+    const errsub = errs.subscribe(setError)
+
+    return () => {


Why didn't you add these as child subscriptions?

Totally possible. Really just a style/code maintenance thing.

It's probably because I primarily used rxjs for a somewhat different use case I have cognitive bias to keep my streams/observables very separate.

grampelberg · 2020-08-20T20:23:52Z

hooks/useFetch.js

+    }
+
+    const status = all.pipe(
+      map((url) => {


sourishkrout · 2020-08-21T15:32:51Z

Getting there! I'll take a look later today or over the weekend. :-D

sourishkrout · 2020-08-22T20:34:24Z

@grampelberg the one last thing I noticed is that refreshers will halt (complete) on error - is that what you want? if it should continue to refresh on error the error handling needs to be moved higher in the order before switchMap

sourishkrout added 2 commits August 17, 2020 21:54

rxjs idiomatic useFetch

f76f41a

rxjs idiomatic useFetchMany

9c52891

grampelberg reviewed Aug 18, 2020

View reviewed changes

Toggle loading, separate out errors, all streaming

d9126ef

sourishkrout requested a review from grampelberg August 20, 2020 20:03

grampelberg reviewed Aug 20, 2020

View reviewed changes

Properly toggle loading on refresh

84660fd

sourishkrout requested a review from grampelberg August 22, 2020 20:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Idiomatic rxjs #1

Idiomatic rxjs #1

sourishkrout commented Aug 18, 2020

grampelberg left a comment

grampelberg Aug 18, 2020

sourishkrout Aug 18, 2020

grampelberg Aug 18, 2020

sourishkrout Aug 18, 2020

grampelberg Aug 18, 2020

sourishkrout Aug 18, 2020

sourishkrout Aug 18, 2020

grampelberg Aug 18, 2020

sourishkrout Aug 18, 2020

grampelberg Aug 20, 2020

sourishkrout Aug 22, 2020

grampelberg Aug 18, 2020

sourishkrout Aug 18, 2020

grampelberg Aug 18, 2020

sourishkrout Aug 18, 2020

grampelberg Aug 18, 2020

sourishkrout Aug 18, 2020

grampelberg left a comment

grampelberg Aug 20, 2020

grampelberg Aug 20, 2020

sourishkrout Aug 22, 2020

sourishkrout Aug 22, 2020

grampelberg Aug 20, 2020

sourishkrout Aug 22, 2020

grampelberg Aug 20, 2020

sourishkrout commented Aug 21, 2020

sourishkrout commented Aug 22, 2020

Idiomatic rxjs #1

Are you sure you want to change the base?

Idiomatic rxjs #1

Conversation

sourishkrout commented Aug 18, 2020

grampelberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grampelberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sourishkrout commented Aug 21, 2020

sourishkrout commented Aug 22, 2020