MartinMalinda / vue-concurrency

A library for encapsulating asynchronous operations and managing concurrency for Vue and Composition API.
https://vue-concurrency.netlify.app/
MIT License
350 stars 15 forks source link

Promise.all() behaviour on tasks #29

Closed gerardnll closed 3 years ago

gerardnll commented 3 years ago

Hello, I'm having some issues getting some parallel tasks to work. The code in the Promise.all().then() is called immediately, it does not wait for the commit on the action. I thought the 'then' callbacks were called one after another and that 'yield' should wait for it. Here's the example:

setup() {
    const store = useStore(); // https://vueuse.js.org/

    const getUserTask = useTask(function* () {
        yield store.dispatch('auth/getUser');
    }).drop();

    const getBusinessTask = useTask(function* () {
        yield store.dispatch('business/getBusiness');
    }).drop();

    Promise.all([getUserTask, getBusinessTask]).then(() => {
        console.log('all resolved');
    });
}

And one of the stores actions (the other one does the same on another endpoint) look like this (in reality, the axios call is located in another file, that's why 2 'then' callbacks):

getBusiness({commit}) {
    return axios.get('/back/api/business').then(function (response) {
        return response.data.data;
    }).then((business) => {
        commit('setBusiness', business);
    });
}

Have I misunderstood something? Thanks a lot.

MartinMalinda commented 3 years ago

Hey @gerardnll . It looks like you're only defining tasks, but not performing them. Calling useTask will not call the generator function right away.

.perform() returns a TaskInstance which is thenable, so it can be used as a promise.

So you're probably looking for something like this:

 Promise.all([getUserTask.perform(), getBusinessTask.perform()]).then(() => {
        console.log('all resolved');
    });

But then you'd probably have to do something like getUserTask.isRunning || getBusinessTask.isRunning, same with error etc.

So there's two ways to simplify this:

1) Create yeat another wrap tasks, that performs the child tasks:

const fetchDataTask = useTask(function * () {
 yield Promise.all([getUserTask.perform(), getBusinessTask.perform()])
});
fetchDataTask.perform(); // perform right away

// or like this for short
const fetchDataTask = useParallelTask([getUserTask, getBusinessTask]);
fetchDataTask.perform();

2) Task Group

// perform both right away in setup
getUserTask.perform();
getBusinessTask.perform();
const getData = useTaskGroup({ getUserTask, getBusinessTask });
// task group only tracks state, so tasks are performed on their own
console.log(getData.isRunning);
// getData.isError does not work so far, maybe I should add it 🤔 

Hope this helps!

gerardnll commented 3 years ago

Ups, I forgot the .perform() on the code examples, but anyway... I was calling them outside the Promise.all([]) so wrong because I have to pass the instance not the task. OK.

What I'm looking for is just an initial load of some data. Would this be OK?

const getUserTask = useTask(function* () {
    yield store.dispatch('auth/getUser');
}).drop();

const getDeliveriesTask = useTask(function* () {
    yield store.dispatch('deliveries/getAllDeliveries');
}).drop();

const getBusinessTask = useTask(function* () {
    yield store.dispatch('business/getBusiness');
}).drop();

const fetchDataInitialLoad = useParallelTask([getUserTask, getDeliveriesTask, getBusinessTask]);

fetchDataInitialLoad.perform().then(() => {
    initialLoad.value = true;
}).catch(() => {
    initialLoadError.value = true;
});

But when I run this code it doesn't work. Looking at the parallel task, there's one object in _instances and it has an error: 'Cannot read property 'apply' of undefined'. Kind of a weird error. It does not even fire the ajax calls.

Captura de Pantalla 2020-12-11 a les 21 25 50

It all works correctly if I use something like this:

Promise.all([store.dispatch('business/getBusiness'), store.dispatch('deliveries/getAllDeliveries'), store.dispatch('auth/getUser')]).then(() => {
    initialLoad.value = true;
}).catch(() => {
    initialLoadError.value = true;
});

Thanks a lot Martin!

MartinMalinda commented 3 years ago

oops, sorry! It's not meant to be in an array: https://vue-concurrency.netlify.app/composing-tasks/#parallel-task

.apply() error then probably comes from Babel, which transpiles the spread ... logic the parallel tasks uses.

I should probably change this to accept arrays though 🤔

gerardnll commented 3 years ago

It still gives me the same error even if I remove the array. What am I doing wrong? (Vue 3.0.4)

MartinMalinda commented 3 years ago

I've just added an extra test to cover your case (parallel task without arguments) and it passes in Vue 2 and Vue 3.

      const firstTask = useTask(function*(signal) {
        return 1;
      });
      const secondTask = useTask(function * (signal) {
        return 2;
      });
      const parallelTask = useParallelTask(firstTask, secondTask);
      const [first, second] = await parallelTask.perform();
      expect(first).toBe(1);
      expect(second).toBe(2);

Not sure if I can help further without minimal repro.

Do the other approaches work? The wrap task approach or the task group approach? I think they're not much worse than the useParallelTask.

gerardnll commented 3 years ago

I finally got it working doing:

Promise.all([getUserTask.perform(), getDeliveriesTask.perform(), getBusinessTask.perform()]).then(() => {});

Works for me as I don't need to track status. The same code as a parallel task wasn't working. I didn't try the Task group.

Thanks for your help! Awesome package.

MartinMalinda commented 3 years ago

Great!