Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support listenerApi.joinForks() or similar. #3313

Closed
ericanderson opened this issue Mar 31, 2023 · 4 comments · Fixed by #3407
Closed

Support listenerApi.joinForks() or similar. #3313

ericanderson opened this issue Mar 31, 2023 · 4 comments · Fixed by #3407

Comments

@ericanderson
Copy link
Contributor

I am examining converting a lot of existing sagas to the listener middleware and I am very excited.

However, I have come across a few gaps that make things challenging. One of those gaps, is that forks are attatched in sagas and there is no such attachment in the listeners. So where no management of forks is needed when writing sagas, you now need to keep track of your forked results so you can await them explicitly.

function* doThingSaga(action) {
  // lots of code
  yield fork(doAnotherThing);
  // lots of code
  yield fork(doAnotherThing2);
  // lots of code
  if (someCondition) {
    yield fork(doAnotherThing3);
  }
  // lots of code
  if (someCondition2) {
    yield fork(doAnotherThing4);
  }
}

This effectively requires the code:

function doThingEvent(action, api) {
  const forks = [];
  // lots of code
  forks.push(api.fork((forkApi) => doAnotherThing(forkApi, api)));
  // lots of code
  forks.push(api.fork((forkApi) => doAnotherThing2(forkApi, api)));
  // lots of code
  if (someCondition) {
    forks.push(api.fork((forkApi) => doAnotherThing3(forkApi, api)));
  }
  // lots of code
  if (someCondition2) {
    forks.push(api.fork((forkApi) => doAnotherThing4(forkApi, api)));
  }

  await api.pause(Promise.all(forks.map(f => f.result)));
}

In a perfect world, the forks would not stop just because the parent completed, but given how breaking that is, a more helpful alternative could be:

function doThingEvent(action, api) {
  // lots of code
  api.fork((forkApi) => doAnotherThing(forkApi, api));
  // lots of code
  api.fork((forkApi) => doAnotherThing2(forkApi, api));
  // lots of code
  if (someCondition) {
    api.fork((forkApi) => doAnotherThing3(forkApi, api));
  }
  // lots of code
  if (someCondition2) {
    api.fork((forkApi) => doAnotherThing4(forkApi, api));
  }

  await api.joinForks();
}

This is much cleaner/easier to read.

@markerikson
Copy link
Collaborator

I really haven't used sagas and forking myself, so I don't have the background to compare here.

What would be the expected semantics and behavior for a .joinForks() method?

@ericanderson
Copy link
Contributor Author

Internally creating the fork would keep track of the 'child'. Currently this is implicitly done via https://github.com/reduxjs/redux-toolkit/blob/master/packages/toolkit/src/listenerMiddleware/index.ts#L82-L85

However instead there could be a direct reference in the parent to all created children and joinForks() could be similar to the above api.pause(Promise.all(forks.map(f => f.result)));

I would be happy to provide a demonstration of the change if it is something you are open to, otherwise I will save my time.

@markerikson
Copy link
Collaborator

@ericanderson sure, definitely interested!

@ericanderson
Copy link
Contributor Author

ericanderson commented Apr 27, 2023

Updated version of proposed solution with #3407

function doThingEvent(action, api) {
  // lots of code
  api.fork((forkApi) => doAnotherThing(forkApi, api), { autoJoin: true });
  // lots of code
  api.fork((forkApi) => doAnotherThing2(forkApi, api), { autoJoin: true });
  // lots of code
  if (someCondition) {
    api.fork((forkApi) => doAnotherThing3(forkApi, api), { autoJoin: true });
  }
  // lots of code
  if (someCondition2) {
    api.fork((forkApi) => doAnotherThing4(forkApi, api), { autoJoin: true });
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants