async reducers

[boomfly]

No description provided.

[harryadel]

How wasn't this already detected by the tests? Maybe you're ought to address this too so we can prevent future regressions. Otherwise, great work 👏

[bhunjadi]

@boomfly Thanks for the PR! From what I see the tests are failing and we have a potential breaking change if we want to solve this issue.

@harryadel There were no async reducers in tests, that's why everything worked before.

Now, back to the issue:

1. You didn't await here in https://github.com/boomfly/grapher/blob/6aa7bdd829f6bd09288ba48f1adf80395bbea069/lib/query/lib/recursiveFetch.js#L97
2. This has consequences here in https://github.com/boomfly/grapher/blob/6aa7bdd829f6bd09288ba48f1adf80395bbea069/lib/query/query.client.js#L217 and also in https://github.com/boomfly/grapher/blob/6aa7bdd829f6bd09288ba48f1adf80395bbea069/lib/namedQuery/namedQuery.client.js#L218, where it makes those methods async

The problem is that in case of reactivity we probably don't want query.fetch() to be async. As far as I understand the purpose of subscriptions, you want to be using sync methods in your code and refreshes are triggered when underlying collections change allowing you to use sync methods. The other issue I see is I don't want to change the client API because of this implementation detail.

As an example, look at this https://github.com/boomfly/grapher/blob/6aa7bdd829f6bd09288ba48f1adf80395bbea069/lib/query/testing/reducers.client.test.js#L14. On line 15 const data = query.fetch() we're not getting fullName because reactiveFetch does not await.

What we need to do is await in recursiveFetch, but then the question is what do we do with _fetchReactive in (query|namedQuery).client.js?

Option A.
We force using fetchAsync on client and remove the check https://github.com/boomfly/grapher/blob/6aa7bdd829f6bd09288ba48f1adf80395bbea069/lib/query/query.client.js#L79.
As I said above, I don't like that solution.

Option B.
One idea is to handle _fetchReactive in subscribe method. Not sure if this will work in all situations, it has to be tested properly.

 cache = {};

 subscribe(callback) {
    this.doValidateParams();

    this.subscriptionHandle = Meteor.subscribe(
      this.name,
      prepareForProcess(this.body, this.params),
      () => {
        callback?.();


        //  Run this here after all collections are ready.
        // cache the result
        Tracker.autorun(async () => {
          const results = await this._fetchReactive();
          this.cache[this.subscriptionHandle.subscriptionId] = results;
        });
      },
    );

    return this.subscriptionHandle;
  }

// and then we use cache in reactive fetch
fetch(callbackOrOptions) {
    this.doValidateParams();

    if (!this.subscriptionHandle) {
      this._fetchStatic().then(
        (res) => {
          callbackOrOptions(undefined, res);
        },
        (err) => {
          callbackOrOptions(err, undefined);
        },
      );
    } else {

      // Use cache instead of async _fetchReactive
      return this.cache[this.subscriptionHandle.subscriptionId];
      // return this._fetchReactive(callbackOrOptions);
    }
  }

// Also, clear it in unsubscribe
  unsubscribe() {
    if (this.subscriptionHandle) {
      this.cache[this.subscriptionHandle.subscriptionId] = null;
      this.subscriptionHandle.stop();
    }

    this.subscriptionHandle = null;
  }

I'd like your thoughts on this matter @harryadel @boomfly . I can also help in the implementation of this PR if you're not confident enough @boomfly . We just need to have a clear path on what do we want to achieve.

I'll also add better local testing procedure in my update/meteor-v3.0 branch
EDIT: please check https://github.com/bhunjadi/grapher/blob/update/meteor-v3.0/README.md#local-testing for local testing instructions.

[harryadel]

You kinda outlined the possible paths here @bhunjadi. Personally, I'm all for backwards compatibility whenever it's possible so if you could find a way for the client side code to function as is while the server side becomes async I'd encourage you to go for.

If not, I don't also mind changing the client side because given the spirit of 3.0, I think it's fair to pay a little price in terms of modifying the code for the upgrading your application. I think you'll have a clear idea once you get down to it and try to actually implement it.

The only thing which is an absolute must for me is having tests so we can move forward more confidently regardless of the path we choose.

[bhunjadi]

Went with option B, closing this MR.
We can continue discussion here: cult-of-coders#484