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: e37ba55e0507ad1b80afd3fcfa6e3a551fe4d1a3.

[rijk]

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

[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 :grin:

[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 )