Effects and memoization

[joshribakoff]

Ideally subscription should happen in commit phase not render phase, subscribing to an observable is like creating a promise in the sense it’s a side effect, it shouldn’t happen in render phase, where react may render numerous times before committing

[kosich]

Hey, Josh! Good point, thanks!
I agree, subscription is heavy and generally should be put outside the rendering.

The idea here was to have a value present in the first render if Observable is, e.g. of(<img />) or has startWith('Loading...'). Thus we avoid re-renders and the components should be compatible with SSR (in the future)

Here's (roughly) how I imagine SSR:

function App(){
  return <$>{
    fetchAPI().pipe(
      startWith(SUSPENSE), // < declare that we'll use React.Suspense
      map(r => <Child data={ r.data } />),
      catchError(e => of('heck'))
    )
  }</$>
}

I can't think of a way to achieve this w/o subscribing in the first render. Maybe I'm missing something.

The alternative is to have SSR attribute to be enabled declaratively, e.g.:

function App(){
  return <$ suspense>{  // < declare that we'll use React.Suspense
    fetchAPI().pipe(
      map(r => <Child data={ r.data } />),
      catchError(e => of('heck'))
    )
  }</$>
}

For no good reason, currently, I prefer first approach 🙂 While totally open to discussion!

Please, let me know what you think!

[joshribakoff]

I see how you arrived at this and I was sort of assuming this was the case...

The thing is with the render phase react really wants to be able to abort that without any cleanup. If react aborts renders, subscriptions made therein will not be cleaned up because react never commits and therefore the destroy$ stream in shared.ts will not fire and subscriptions will remain active which correspond to “aborted” react trees.

For a while, react-redux caused cascading renders, I think they may have solved it with hooks because redux uses synchronous state accessed via a pojo. With rxjs we are working with more of a callback vs reading from state. For SSR see useSelector. They use useIsomorphicLayoutEffect..

There is an impedance mismatch here between rxjs and react for sure, and I don’t have perfect solution that would optimize all the tradeoffs ... I think your solution is really nifty for sure. Maybe there is a way to decouple the tradeoffs, eg. Make an alternative api that is more compatible with async react, or solve the initial render issue via some other mechanism or library? I’ll be giving this impedance mismatch some deep thought on my end as well

[kosich]

Very important remark regarding mem-leak, will definitely fix!

For SSR see useSelector. They use useIsomorphicLayoutEffect..

Uhuh, I've seen a similar approach with the forceRerender hack already. Not sure how reliable that hack is with future React updates.

Make an alternative api that is more compatible with async react, or solve the initial render issue via some other mechanism or library?

Currently, I see 3 ways to fix this (in current codebase or 3rd party lib):

1. use the forceRerender hack (probably unreliable)
2. use React.Suspense under the hood (probably slow)
3. use useEffect and drop the SSR/rerendering issue altogether

well, number four is a mix

4. use a number of flags to turn on/off above-mentioned

Am I missing something else?

--

Will give it a thought and play around with the code and approaches next week (currently on vacation 🙂)

Thanks, Josh!

[joshribakoff]

Nice, enjoy the vacation!

What do you mean by forceRender being unreliable? what are its failure modes you're referring to?

I think different APIs for subscribing in different contexts makes sense. Eg. different fragment component for subscribing in an effect, layout effect, iso layout effect, render phase (if this is something you want to support), and suspense (throws a promise that resolves when the obs emits?) like we were talking about on Twitter?

Will be interested to see how you solve the memory leak issue with subscribing in render, I personally think it will be challenging because React isn't designed for this exact use case.

[kosich]

What do you mean by forceRender being unreliable? what are its failure modes you're referring to?

I stupidly mistook it twice for other way around: forcing commit from render phase 😕 . So I thought its a "hack" that is unstable.

Will be interested to see how you solve the memory leak issue with subscribing in render, I personally think it will be challenging because React isn't designed for this exact use case.

Yep, I think React is ideologically against that. By "fixing" I meant reworking the API to have a stable & non-leaking subscription.

in an effect, layout effect, iso layout effect, render phase (if this is something you want to support), and suspense (throws a promise that resolves when the obs emits?)

Probably will keep the effect phase subscription with suspense as an option. I haven't had a chance to investigate the use-cases for other subscription timing (or applying updates). While SSR is an obvious requirement, I'd like to postpone other options til community asks for particular real-life scenario (if you already have something -- I'd be happy to discuss that in a dedicated issue).

I guess, you're also considering extending rx-store's useSubscription() with something above-mentioned? Please, ping me if you'll have new thoughts/updates!

[joshribakoff]

I apologize in advance for cross posting about my lib on your lib, hope its okay, but the approach I'm playing around with is using a useResource hook w/ a BehaviorSubject and a convention of using scan to build hash maps with immutable values https://github.com/rx-store/rx-store/pull/69/files#diff-95f7c390f2d0664110959b4242d0ce4dR179-R207

As noted there, the value returned by the hook is still subject to tearing if you don't use a hashmap with immutable objects, so if you need to support a non immutable data structure, you can only use the hook to suspend, and the tradeoff you'd have to accept is the first render would contain placeholder data only, until the useSubscription runs after mount & triggers a second render.

But as long as you are using Suspense in the "traditional" manner, for fetching "resources" it will work. Think of it like this, a Promise represents some resource that will resolve to some immutable value at some future point in time. In the same vein, this useResource hook I'm proposing would also need to be used in a way where the resource is some immutable value that resolves once (even though its technically backed by a stream which technically lets you change the value over time)

Also feel free to close this if you want, but I would suggest considering adding something to your docs or readme about the tradeoffs the library makes in this regard if you are keeping the current implementation (totally up to you 😄 ). The reason for making the issue was I'd like to "offload" the consuming of streams to 3rd party libraries that I recommend to users of Rx Store, but I want to make sure if I recommend a library, it fully discloses all the "potential gothyas" & tradeoffs

[kosich]

Hey, Josh!

It took me long to address this, sorry 😓
And surely it was impossible to close this as the issue was very important!
Thanks for reporting this and guiding on details! 🙏

The 0.0.5 release switches back to useEffect strategy (with a bunch of other fixes and optimisations).
And I will investigate the path you advice on useResource / BehaviorSubject approach in #3 !

Please, feel free to re-open this issue if you see it's not resolved.

Thanks for contributing! ❤️