Try: Collect async resolver calls

[psealock]

@joshuatf I'm trying to implement #4144 (comment) and running into difficulties.

Any suggestions here?

I created a getOption selector and resolver to accept a single option name as an argument. I also made changes to withOptionsHydration to mark any preloaded options as resolved. Everything is working nicely.

I'm having trouble with the collection of requests you suggested. The function in the setTimeout needs to be a generator function (or maybe just async?) to respect keywords like yield.

Directions

• Head to the homepage and see the undefined options.
• Change the requests to preloaded options such as woocommerce_task_list_do_this_later and woocommerce_task_list_hidden and see the hydration happening correctly.

[joshuatf]

This is really cool to have in place and looking forward to how much this will simplify fetching options! 💯

The function in the setTimeout needs to be a generator function (or maybe just async?) to respect keywords like yield.

This was significantly trickier than I originally imagined. I hope you don't mind that I pushed a commit here ( e5c3573 ) since I wasn't sure I could quite describe the process. Feel free to drop/add/tweak as you see fit.

The problem

We needed to wrap the timeout in a promise and resolve when complete. However, it seems like wp data wants a dispatch action/control object before any yield. It will await the control's response, but yield will not await a promise directly in any resolver.

Solution

I added a control that's sole responsibility is to get the names. That wraps the timeout in a promise and resolves when complete. There's also a timeout inside of that to clear the option names for future requests (though there's probably a more elegant solution involving saving those option names in the state).

The yield will wait for the control promise to resolve and then begins the fetch if one doesn't exist.

I also removed the returns since I don't think those will have an effect similar to the comment I made here - #4433 (comment)

Possible improvements

• As mentioned above, I think storing optionToRequest in the store may be a slightly better solution, but I'm not sure if that introduces more problems and don't see the need to do this at the moment.
• We probably need a way to handle invalidation on those fetches. Since wp data comes with invalidateResolution, this might suggest also moving those fetches to the store so invalidation can be handled there. Edit: I think we can just delete the fetch after the request is resolved to solve this - 47f149e

[psealock]

🙇 🧙‍♂️ Thanks for the help here @joshuatf

Nice one, that is a clever solution! Its working well and collecting names before making a request.

One issue I can see is the early return in the resolver if statement, fetchInProgress === true, will cause isResolving to be false on some of the options when requests are still actually being made.

In c01969f I added some console.log's to see what was happening and modified the if statement to yield to the fetch in progress as well.

Before, isResolving was returning false too early, now the opposite is true. But thats arguably better because any logic waiting on isResolving won't get a false with no data.

What do you think? Is there a better way?

[joshuatf]

Just a heads up that I added this back in since we need to return early here.

Returning yield apiFetch( { path: url } ); will hit the options endpoint again for each request.

[nerrad]

As Paul pointed out, this will cause the built in cached resolver for getOption( name ) to be marked as finished - so this effectively means you cannot depend on the wp.data resolution state for any of the options to be accurate. This could be a gotcha for folks not realizing getOption has a special resolution state.

[joshuatf]

One issue I can see is the early return in the resolver if statement, fetchInProgress === true, will cause isResolving to be false on some of the options when requests are still actually being made.

Great eyes @psealock! I didn't see that before.

I guess the resolution status is marked as complete once the resolver finishes going through the generator (which happens much faster in the option that gets returned early).

I looked at overriding the isResolving selector so we could manually call finishResolution(), but could not find a way to do this without breaking the baked in resolution state. I think it may be best here to just handle in our own isRequesting selector which I added in 6c19881. This marks the collected options as complete when receiveOptions is called.

[psealock]

I added error handling in f24f603

I think it may be best here to just handle in our own isRequesting selector

yeah, that works. It is a bit unfortunate to not have a uniform pattern across the dataStores. In fact, this brings us back to the original problem that spawned this investigation. A new selector was required to discern if options are resolving or not, and we tried this branch to avoid that problem.

Any opinion why one method is better than the other?

[joshuatf]

yeah, that works. It is a bit unfortunate to not have a uniform pattern across the dataStores.

I agree. This is not a dealbreaker for me in this PR but it would be great if there was a way we could override the built in resolution status. Maybe @nerrad knows a trick for doing this.

Any opinion why one method is better than the other?

I definitely think this branch is an improvement. The ability to individually get options, even from separate components, and only have one API request made is enough of an improvement in my book. Plus it simplifies the logic for checking if an option is requesting:

// Previously 
updateOptions( { optionA: ..., optionB: ... } );
isRequesting( [ ‘optionA’, ‘optionB’ ] );  // true
isRequesting( [ ‘optionA’, ]);  // false
isRequesting( [ ‘optionB’ ]);  // false
isRequesting( [ ‘optionB’, ‘optionA’ ]);  // false

// Now
isRequesting( ‘optionA’ ); // true
isRequesting( ‘optionB’ ); // true

[nerrad]

This is clever and I like how you are collecting option names to create a batched request!

However, the significant downside in my opinion is that the wp.data resolution state for each option now can't be trusted. While internally this may be okay because you folks can note it and work around it, it means that it diverges from the known api for tracking resolution (something that is already a bit hard to pick up by those new to wp.data).

So what to do here?

One thing that came to mind that you could try is to move all the logic about the fetch and the batch setup into your custom control. That way the control itself is absorbing all the object tracking etc and only resolves when the fetch is complete. Then the api for your resolver would just be something like:

export function* getOption( name ) {
    try {
      // `batchFetch` could be abstracted so the first argument can be used to derive what endpoint to
      // hit. That way you could potentially use this new batch fetch control for different endpoints.
      const { result, optionNames } = yield batchFetch( 'option', name );
      yield receiveOptions( result );
    } catch (error) {
      yield setRequestingError( error, optionNames );
    }
}

By doing things this way, the control will only yield when the batch fetch is complete and all your timeouts are absorbed by the control itself. So you retain the resolution state built in for each option in wp.data while still doing a batched request.

Thoughts? Anything you need elaborated on?

[nerrad]

This comment is a bit ambiguous. Are you talking about wp.data handling resolver cache invalidation here? If true, then no this won't handle resolver cache invalidation.

[joshuatf]

Up for rewording this comment, but I believe it's accurate. We need to delete the fetch from our local variable to allow (keyword) wp data to handle invalidation. It does not invalidate cache directly, just allows it.

If we don't remove this fetch and we want to invalidate this cache using invalidateResolution, it would never actually work since the previous promise has already resolved.

[nerrad]

You could probably just eliminate the declaration of fetchInProgress here and use fetches[names] directly in the condition.

[nerrad]

As Paul pointed out, this will cause the built in cached resolver for getOption( name ) to be marked as finished - so this effectively means you cannot depend on the wp.data resolution state for any of the options to be accurate. This could be a gotcha for folks not realizing getOption has a special resolution state.

[joshuatf]

By doing things this way, the control will only yield when the batch fetch is complete and all your timeouts are absorbed by the control itself. So you retain the resolution state built in for each option in wp.data while still doing a batched request.

This is a great idea @nerrad. I'll take a quick whack at it today and see if I can't get something working.

[joshuatf]

Okay, got this working by moving the fetch logic into the control as @nerrad suggested (thanks for the suggestion!). Resolution status seems to be working, though you can see that one option resolves first in this case since its promise is resolved at the end of the stack so I think this is expected behavior.

I also didn't abstract the batch fetch function yet, though I agree this is probably a good idea. But we can always tackle this in a follow-up when we need it.

@psealock I removed some of the isRequesting logic I added before, but didn't want to hijack your PR too much. Happy to help clean things up a bit more, just not sure how much of that logic is needed for other areas of this store.

[psealock]

Thanks for the awesome help here @joshuatf and @nerrad !

I cleaned this up, added some tests and doc blocks. I'll merge this to the original PR and continue with the refactor and rebase.

[nerrad]

I'm not sure this will be entirely accurate. With the batch fetch, there's the potential for multiple names to be in the request that causes the error right? Would you want that reflected in the error state? That's why in my example I suggested yielding an object from the batchFetch control: { result, names } so you have all the option names that were used in the actual request.