Data Module: Adding support for sync generators to allow async action creators

[youknowriad]

an alternative to #6999

This PR refactors the data module to favor sync generators in favor of async ones:

Pros:

• This allows us to know exactly when a generator starts which means if the same generator is executed on the same "tick", we can stop the second one because the first one is already in progress.
• Sync generators using a library like rungen allows a descriptive flow :
  • Generators describe the flow without actually running the side effects (network calls, dispatches...)
  • The generator runtime controls (pauses and resumes) the execution of this flow.
    (Details on this blog post https://riad.blog/2015/12/28/redux-nowadays-from-actions-creators-to-sagas/)
• Sync generators don't require mocks to be tested because the side effect is not run inside the generator itself.

Cons:

• A bit more complex to understand

[youknowriad]

It's not possible to support this usecase in sync generators. Because of this:

imagine the promise returns an object that's not an action, and that object has a "type" property (we have this use case in posts, if we try to fetch a post, we'd receive a promise of an object with a type property but it's not an action), there's no way to know whether we should return this object or dispatch it instead.

In general redux-saga and redux-rungen solve this issue by requiring sync generators to wrap action inside helpers to avoid ambiguity:

function *() {
   yield Promise.resolve( put( { type: 'SET_OK' } ) );
}

The put helper here doesn't do anything special aside wrapping the given action in an object that can't be ambiguous. We're certain it's an action, so we dispatch the action and not return the object as the yielded result.

[tofumatt]

For what it's worth: getting used to doing that was very easy when we moved to using redux-saga on the addons-frontend.

[tofumatt]

I like this a lot (and it reminds me of sagas, which is probably why I like it).

Based on some code changes I see below, because the functions themselves are no longer async, does this mean always using yield in place of await for things (like apiRequest) in resolver code? Or only when the generators are sync?

I guess it might just be surprising to a developer to see yield and await in front of the same function calls in different places.

[tofumatt]

For what it's worth: getting used to doing that was very easy when we moved to using redux-saga on the addons-frontend.

[youknowriad]

Based on some code changes I see below, because the functions themselves are no longer async, does this mean always using yield in place of await for things (like apiRequest) in resolver code? Or only when the generators are sync?

Yes, so the idea is that the runtime running the generators (rungen in this case) is configurable and can "respond" to "yielded values".

• If the yielded value is an action, the runtime will dispatch it
• If the yielded value is a promise, the runtime will resolve it and pass the result to the generators (that's what is causing the change from await to yield when moving to these sync generators)

Rungen allows custom "controls" (or custom yielded values with custom behaviors) which means we can push this further and define special controls to deal with network calls for instance or any side effect call. Similar to call in redux-saga we can introduce a call control so we'd be able to do

function* () {
  const result = yield `call( apiRequest, ...args )`
}

to avoid calling the side effects entirely in the generators. I didn't want to introduce this now, just get the basis and see how to move forward.

[tofumatt]

Cool, that's a great explanation and that makes sense 👍

Might be worth including some of (all of?) that text in a doc explaining things. That was really helpful.

[aduth]

So I'm a bit confused about our intended usage here. In this case, the promise returned by apiRequest does not return an action object, yet we're yielding it the same as we would for any other action. I suppose as implemented, it's up to the runtime to determine whether it "looks" like an action to decide whether it should be dispatched? Not always clear at a glance whether the thing being yielded is an action or not.

I guess in my mind, I had imagined something more like:

const categories = yield { type: 'API_REQUEST', path: '/wp/v2/categories?per_page=-1' };

i.e. always yielding a proper action to which we can attach specific behaviors (through some API to add controls to rungen dynamically). I think this could also help rein in control to prevent abuse of promises by limiting the control flows exposed.

[youknowriad]

const categories = yield { type: 'API_REQUEST', path: '/wp/v2/categories?per_page=-1' };

I think that's the goal, I won't consider these "actions" but "effects". I didn't want to introduce new effects on the initial implementation.

Also, I tend to think we shouldn't couple the data module to API requests. I'd prefer something more generic:

yield { type: 'CALL', function: apiRequest, args: { path: '/wp/v2/categories?per_page=-1' } }

I think it's possible to keep the data module REST API agnostic and still allow you to do what you propose, but this requires having the ability to provide custom controls to the rungen runtime when calling registerStore, not sure how I feel about it. I tend to prefer starting small

[tofumatt]

Also, I tend to think we shouldn't couple the data module to API requests. I'd prefer something more generic:

I know this isn't a democracy, but I agree 😆

Keeping the module API-agnostic means testing/mocking is easier and it's easy to allow people to use their own functions to intercept/modify certain calls if wanted. Win/win in my mind.

[aduth]

We've already promoted resolvers as being the swappable layer of abstraction, so do we really have to go the next step further and also make its yielded actions generic?

[tofumatt]

I suppose not, but I find there's a lot of flexibility in this approach.

If I wanted to test the generic approach, it would be easier, for instance.

I suppose I don't see the harm in it but I see benefits.

I'm not sure what the "abuse of promises" would be as mentioned earlier, but I'm generally in favour of us giving ourselves lots of flexibility. I know flexibility is another word for footgun, but I guess I don't see what the danger is here. 🤷‍♂️

[aduth]

I know flexibility is another word for footgun,

This many times.

I've often pointed to the effects.js file as a prime example of flexibility gone wrong, as the file has been a failure in almost every regard: Lack of self-contained documented functions, non-obvious behaviors, callback hell, side effects which aren't side effects, implicit behaviors by reaching into state.

[tofumatt]

For what it's worth though, the proposed, saga-style pattern is more idiomatic and in my experience using something similar, never bit us or made us write confusing/dangerous code on the Add-ons project.

The effects are definitely a bit too out there. I don't think what's being proposed is as flexible or dangerous, and I think perhaps some intense code review around the first set of patches could lead to us creating strong idioms we could document and stick with.

[dmsnell]

holy cannoli.

function* foo() { yield new Promise() }

and oh how nasty this is without strong static types,
without the compiler helping make sense of it.

just had to toss that out there 😉

[aduth]

Also, I tend to think we shouldn't couple the data module to API requests. I'd prefer something more generic:

In talking about this with @youknowriad today, I came to realize I misinterpreted the point here. I never meant to suggest that the data module should have awareness of how to make API interactions, but rather that it should provide the tools to allow an individual store to define its own continuation procedures for specific action types, so if the core-data store decided it needed a way to make API calls, it could define this through some API exposed on the data module (the generalized "controls" structure).

Rough idea:

registerStore( 'core', {
    // ...
    
    controls: {
        async API_REQUEST( store, next, action ) {
            next( await apiFetch( action.path ) );
        },
    }
} );

Unsure if it needs to be middleware-esque, or just return the Promise:

registerStore( 'core', {
    // ...
    
    controls: {
        API_REQUEST( action, store ) {
            return apiFetch( action.path );
        },
    }
} );

Then what we have here becomes:

export function* getCategories() {
	const categories = yield { type: 'API_REQUEST', path: '/wp/v2/categories?per_page=-1' };
	// OR: yield apiRequest( { path: '/wp/v2/categories?per_page=-1' } );
	//     ... where `apiRequest` is a locally-defined action creator
	yield receiveTerms( 'categories', categories );
}

It's a small difference, but intentionally restricts what's possible in the action creators alone to avoid the footgun effect of allowing unrestricted promises, encouraging instead a common set of well-defined continuation conditions. Further, it's more consistent in that the only thing that's yielded are action objects.

[atimmer]

I am a bit confused about this line: yield apiRequest( { path: '/wp/v2/categories?per_page=-1' } );. I thought that one of the advantages of redux-saga is that it is easier to test. But in this case, the function immediately yields a promise with a side-effect. Is that simply because you didn't wrap the call inside a special function like in the redux-saga examples?

[youknowriad]

superseded by #8096