Attempt to make the useSubscription hook simpler by removing reactivity

[rijk]

This is a concept, work in progress based on @yched's comments here and here.

This changes the useSubscription signature to match Meteor.subscribe. To be used as follows:

Normal:

const handle = useSubscription('board.projects', boardId)

Conditional:

const handle = useSubscription(!!boardId && 'board.projects', boardId)

This branch also contains a fix/workaround for the issue that the previous subscription's status is returned for one render after changing the arguments: e37ba55.

[rijk]

@yched would love your review on this if you have time.

[yched]

I don't think we should put that in state : we don't need to rerender when the subscription changes
(the fact that the subscription changes will probably cause other rerenders because of minimongo results changing, but that is taken care of outside of here)

A useRef should be enough ?

[rijk]

Hm, that's a really good point. We don't need a rerender for a changing subscription. The ready state might change, though. I'll change it to a ref and see if I can get the ready state to invalidate immediately as well.

[yched]

the ready state might change, that is handled by the ready() func being a reactive source.
So the consumer can decide to rerender (if he wants) by doing
const isReady = useTracker(ready)
with the ready function we return ?

[yched]

ah yes, that means the stub ready() getter we initially return must be reactive too :-/
Well, that might require playing with Tracker deps to build our own reactve func wrapping the one in the subscription handler, rather than handing out that one directly

[rijk]

I incorporated the changes in 76943f7.

The problem with ready(), is that it will always lag behind one render. So when changing the deps, it will still return the previous subscription's status for one render. I have a suggestion on how to fix that, I'll push that in the next commit.

[rijk]

Ok so here is a suggested fix for this problem: e37ba55. @CaptainN @yched open to better suggestions 😬

[yched]

I think you don't need to manually track and compare deps here, you can just set subscription.current back to null in the effect cleanup phase ?

[rijk]

Well, the thing is, this all happens after the render. So it will already be too late by then.

[yched]

The cleanup phase runs after the render, but before the ready() callback you return gets executed I think ?
Then by the time it gets executed, subscription.current has gone back to null and you can just return false ?
(not 100% sure off the top of my head, but worth a try I think)

More importantly, this code still has the same drawback as before : by not running the reactive subscription.current.ready() in some cases, the function we build here won't be reactive later ?

I have something in mind, need to try it first :-)

[rijk]

The cleanup phase runs after the render, but before the ready() callback you return gets executed I think ?
Then by the time it gets executed, subscription.current has gone back to null and you can just return false ?

Nope. Tried 10 different ways, even with useState, but every time the first ready() call still references the old subscription.

[CaptainN]

I tried a bunch of iterations with this API before settling on a Factory method. There were just too many tricky issues to deal with, and I didn't want to start doing things like deep array compares. One unresolvable issue is that if you have an object based publication API, you are basically not going to be able to manage deps effectively this way, without requiring the implementor to jump through hoops to keep their refs immutable.

[rijk]

@CaptainN there's a couple other ideas in this PR as well, if you have time to take a look (in fact, switching out the Factory method was not even the main point).

[CaptainN]

It looks like you did a few different things here. One is that the useSubscriptionClient function no longer returns a SubscriptionHandle, but just a single method (the useCallback method). I'm not apposed to that, but it still. has the problem of inadequate deps checks.

[yched]

Right, publications can receive args not just as litterals but also as objects or arrays, and we cant use those directly as hook deps ([] !== []). That's a bummer, cause the factory API is way less nice 😁

[CaptainN]

I completely agree - this API would be nicer. I just can't think of a way to make it work with inline object refs in a clean way.

[rijk]

In terms of usage, I don't see much difference than using useTracker though:

useTracker( () => Meteor.subscribe( 'board', params.id ), [ params.id ] )
// vs
useSubscription( () => Meteor.subscribe( 'board', params.id ), [ params.id ] )

What is the difference, or is it just that it allows for a more future-proof implementation?

[CaptainN]

It's precisely intended to allow for more future proof implementation, with much easier maintenance. But if you expand the example, it looks pretty different.

const MyComponent = ({ slug }) => {
  const sub = useSubscription(() => Meteor.subscribe('board',  slug), [slug])
  const cursor = useCursor(() => MyCollection.find({ slug }), [slug])

  if (sub.ready()) {
    return <Loading />
  } else {
    return <ul>
      {cursor.map(thing => <li key={thing.key}>
        {thing.text}
      </li>}
    </ul>
  }
}

const MyComponent = ({ slug }) => {
  const { isReady, data } = useTracker(() => {
    const handle = Meteor.subscribe('board',  slug), [slug])
    const data = MyCollection.find({ slug }).fetch()
    return {
      isReady: handle.ready(),
      data: data
    }
  }, [slug])

  if (isReady) {
    return <Loading />
  } else {
    return <ul>
      {data.map(thing => <li key={thing.key}>
        {thing.text}
      </li>}
    </ul>
  }
}

The first example gets even simpler if you don't use .ready() or you could put the subscribe higher up on the tree, and use more specific queries lower down. You could do that with useTracker of course but I think it looks cleaner this way.

I'm actually running in to familiar problems with the useCursor hook in the implementation though. I'm going to have to look at some solutions like useMutableSource - which looks precisely like what we need. Sadly, it's in prerelease...

[rijk]

@CaptainN I've been toying around with this for the last days, and there is one problem that I keep coming back to:

The problem with ready(), is that it will always lag behind one render. So when changing the deps, it will still return the previous subscription's status for one render.

I think means this hook needs to manage its own state as opposed to relying on useEffect, as this will always lag one render behind a change in deps. There is no way to immediately clear the subscription, regardless if it's in state or ref.

However, that's not all bad, as it will also allow an out for this issue:

There were just too many tricky issues to deal with, and I didn't want to start doing things like deep array compares. One unresolvable issue is that if you have an object based publication API, you are basically not going to be able to manage deps effectively this way, without requiring the implementor to jump through hoops to keep their refs immutable.

If we manage our state ourselves, we can use any function we like for comparing deps. We can use something like Object.is by default, and even give the user the option to pass his own:

const subscription = useSubscription(
  'page.content', // subscription name (no factory)
  page, // full object passed as argument
  ( a, b ) => a.id === b.id && a.version === b.version // custom compare function
)

Or using an off the shelf compare package:

import equal from 'fast-deep-equal'
const subscription = useSubscription('page.content', page, equal)

WDYT?

[yched]

Thinking a bit more about the much nicer "non-factory" approach and the question of non-litteral args :
That's something the original Meteor.subscribe() code already had to solve, since it is designed to be executed multiple times (typically in a reactive function) and do nothing if the args haven't changed. It thus relies on an internal comparison method that can handle object and array params.
(namely, in ddp-client/common/livedata_connection.js - that's based on EJSON.equals()

In this approach where useSubcription() would be a replacement/wrapper for Meteor.subscribe(), it seems totally reasonable that it would rely on EJSON.equals()-based comparisons, just like Meteor.subscribe() does.

[CaptainN]

Maybe it's a personal style thing, but I try to avoid leveraging compare options other than those build in to React's core hooks. In case they change their implementation, and to keep dependencies light.

That said, we can use useMemo which runs synchronously with render, and accepts deps to get immediate changes. I'll implement a solution based on that.

[CaptainN]

@yched That's a great find. I wonder if this has positive implications for actually creating these (and other computations) in render... It looks at a glance similar to an approach I was toying with for a bit.

[yched]

This seems to work for me :

const useSubscription = (name, ...args) => {
  const subscription = useRef();
  const params = useRef();
  const readyDep = useRef(new Tracker.Dependency());

  const equalsCommitted = (newName, newArgs) =>
    params.current && params.current.name === newName && EJSON.equals(params.current.args, newArgs);

  useEffect(() => {
    const computation = Tracker.nonreactive(() => (
      Tracker.autorun(() => {
        subscription.current = name ? Meteor.subscribe(name, ...args) : null;
      })
    ));
    if (!equalsCommitted(name, args)) {
      readyDep.current.changed();
    }
    params.current = { name, args };

    return () => computation.stop();
  }, [name, ...args]);

  if (subscription.current && equalsCommitted(name, args)) {
    return subscription.current.ready;
  }
  // Return a fake (but reactive) ready() callback.
  return () => {
    readyDep.current.depend();
    return false;
  };
};

Demo code (toggling between no subcription / subscription 1 / subscription 2)

function App() {
  const [value, setValue] = useState(0);
  const ready = useSubscription(value > 0 && 'test_publication', value);
  const isReady = useTracker(ready);
  return (
    <div>
      {value > 0 ? `Subscription with ${value}` : 'No subscription'} - {isReady ? 'ready' : 'not ready'}<br/>
      <button onClick={() => setValue(prev => (prev + 1) % 3)}>Change</button>
    </div>
  );
}
// server-side
Meteor.publish('test_publication', function (param) {
  Meteor._sleepForMs(1000);
  return this.ready();
}); 

[rijk]

This looks good @yched , I tested and it worked for me as well. Moreover, with this line const isReady = useTracker(ready) you leave it to the user if he cares about ready state or not, which could save a couple renders. I used it in a one-liner like this; otherwise the variable names (ready / isReady) are a little weird IMO.

const ready = useTracker( useSubscription( 'board', params.id ) )

Downside of this is of course we still need useTracker!

[CaptainN]

@yched That looks pretty good! I don't think you need to worry about managing deps with equalsCommitted though, since that's already managed within the Meteor.subscribe function. I think that means we can essentially not worry about constantly rerunning the full computation creation/destruction lifecycle and calling Meteor.subscribe many times.

I also think we can return a ready method that will enable reactivity, rather than relying on useTracker (and a second computation). We already have reactivity from the computation created in useEffect, so we might as well use that.

Your example adjusted:

function App() {
  const [value, setValue] = useState(0);
  const subscription = useSubscription(value > 0 && 'test_publication', value);
  const isReady = subscription.ready();
  return (
    <div>
      {value > 0 ? `Subscription with ${value}` : 'No subscription'} - {isReady ? 'ready' : 'not ready'}<br/>
      <button onClick={() => setValue(prev => (prev + 1) % 3)}>Change</button>
    </div>
  );
}
// server-side
Meteor.publish('test_publication', function (param) {
  Meteor._sleepForMs(1000);
  return this.ready();
}); 

[CaptainN]

I have a version of the above working, but what I'm finding is that I'm jumping through a lot of hoops to avoid 1 re-render when ready() changes, and I'm questioning whether it's worth it all to avoid 1 possibly unneeded re-render. Especially since it'll probably coincide with a forced render from useCursor anyway. What do you think?

[CaptainN]

I pushed a version of this to my branch. Check it out and let me know what you think! (I do need to test it - will write tests)

[rijk]

Tried it out, and it seems to be working well. Already loving this API:

// useSubscription( () => Meteor.subscribe( 'boards' ), [] )
useSubscription( 'boards' ) // ✨

And coincidentally I also found this makes my Suspense version a perfect drop-in replacement! Not sure if this was intended, but a nice bonus.

import { useSubscription } from '/lib/react-mongo' // No suspense
// import useSubscription from 'meteor-subscribe-suspense' // Suspense

useSubscription( 'board.projects', boardId )