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

Encapsulated tasks #7

Open
MartinMalinda opened this issue Jun 5, 2020 · 8 comments
Open

Encapsulated tasks #7

MartinMalinda opened this issue Jun 5, 2020 · 8 comments
Labels
enhancement New feature or request

Comments

@MartinMalinda
Copy link
Owner

it seems like there's less need for this with Composition API than on the Ember side, but it still might be useful.

http://ember-concurrency.com/docs/encapsulated-task

@lukaiser
Copy link

lukaiser commented Jul 3, 2020

I am loving the project as I am missing ember-concurrency a lot. Thanks for the effort!

How would you substitute encapsulated tasks with composition API?
I guess I don´t know enough about the composition API to really understand it.
I love encapsulated tasks as it is task instance specific data. How would you expose that otherwise?

Thx for the feedback and all the good work!

@MartinMalinda
Copy link
Owner Author

MartinMalinda commented Jul 3, 2020

Hey thanks for this question, I was wondering about it as well!
I think there's several approaches to this, but I've never actually used encapsulated tasks in ember so I don't feel too eager to come up with a right solution right away.

One easy step towards this would be just bind some object to the task callback. It would allow you to persist some state between performs without reaching outside of the task.

const task = useTask(function * () {
 this.foo = 'bar';
});

Since setup has no this, this seems quite safe to do. There's nothing to confuse this with.

But that's not all what encapsulated tasks do. To also allow initial state and computed values, it would have to prob. look something like this:

const task = useStatefulTask({
  progress: 0
  progressPct: computed() => `${task.state.progress * 100}%`,
  *perform() {
        while (this.progress < 100) {
        yield timeout(500);
        this.progress = this.progress + 0.01;
      }

     return 'done';
  }
});

The difference here would be probably that the state would be nested under state prop to avoid conflicts and to make it easier to type with TS.

Possible implementation could look like this:

export function useStatefulTask(taskContent) {
  const cb = taskContent.perform;
  const _contentWithoutPerform = { ...taskContent };
  delete _contentWithoutPerform.perform;
  const state = reactive(_contentWithoutPerform);
  return useTask(cb.bind(state)).withState(state);
}

But then again - is this really needed with composition API?

In Ember, changing this prevented you from reaching outside the task. So it effectively encapsulated your logic. Here you'd still be free to use any kind of ref or other variable from the outside.

I guess with Composition API the way to encapsulate is to... compose.

export function useProgress() {
 const progress = ref(0);
 const task = useTask(function *() { /* ... */);
 return { progress, task }; 
}

But still, I feel like there could be some benefits to this binding and having task.state. task.state would at least enforce some more consistency / structure.

@lukaiser
Copy link

lukaiser commented Jul 7, 2020

Hey Martin

Thanks for the fast and long feedback.
I am not sure if I understand your solution correctly and if it really would serve the same purpose.

Encapsulated tasks encapsulate a task instance not a task.
Therefor, if I understand the Composition API correctly, you would need an additional setup function for each task that creates the state and computed properties for each task instance.
Computed properties and state for tasks might be interesting too, but that should indeed be possible with the Composition API. But I like the idea of having a task.state.

Let me give you an example for the benefits of encapsulated task:

Think of a task that handles file uploads for you. You pass the task a file and it uploads it to the server.
In your UI you would love to show the number of files remaining for upload and the total remaining file size.
To show the number of files remaining, you can use a computed property that counts the running tasks.
This works with all the concurrency goodness of cancellation and errors. If any task instance gets canceled or has an error, the UI would change.

How about the remaining file size? You could do it the old-fashioned way and create a component state or task state called remainingSize. Whenever you start a task instance, you would add the total size to the state and during the upload you would subtract the progress. But then you would again need to think about cancellation and errors. You would need to subtract the remaining value if an instance gets canceled. You probably would have to track the remaining value specifically if your API doesn't provide it. You would need to be super careful with rounded values. So, that at the end of all uploads remainingSize is´really 0. And much much more.

Encapsulated tasks give you additional task instance state and computed properties. Each instance can have its own remainingSize that is explodes to the outside as isRunning is. Therefor you can have a computed property that just sums up the remainingSize of all the running task instances as we did with the number of uploads remaining. And again, this works with all the concurrency goodness of cancellation and errors. If any task instance gets canceled or has an error, the UI would change.

I hope this makes sense!

@MartinMalinda
Copy link
Owner Author

MartinMalinda commented Jul 12, 2020

Ahh, thanks a lot for these clarifications! I totally misunderstood that the state is encapsulated per instance, not per task as a whole.

Think of a task that handles file uploads for you. You pass the task a file and it uploads it to the server.
In your UI you would love to show the number of files remaining for upload and the total remaining file size.
To show the number of files remaining, you can use a computed property that counts the running tasks.
This works with all the concurrency goodness of cancellation and errors. If any task instance gets canceled or has an error, the UI would change.

This is really a great example and ideal usecase for the encapsulated tasks.

Let me think how I'd go around solving this with Composition API.

Probably the task would have to wrapped in some bigger, custom, data structure.

{
 progress: 0,
 result: ... // TaskInstance
}

Instead of doing perform() directly, you would have some kind of custom function and a couple of refs elsewhere...

const files = ref([]);
const addFile = (file) => {
  const progress = ref(0);
  const taskInstance = uploadTask.perform(progress);
  const fileState = reactive({
    file,
    progress,
    result: taskInstance, // used to check for error and cancelation in the template 
    progressPct: computed() => `${fileState.progress * 100}%`,
    remainingSize: computed() => {}, // some computation using `file` and `progress` here?
  });
 files.push(fileState);
}

In this case, the task would accept a ref and change it as needed during the run. I can see there is some overhead here that the encapsulated tasks could solve though.

But I like the idea of having a task.state

I guess there could be both task.state and taskInstance.state but it could be confusing as well. From my experience beginners are already confused with task.isRunning vs taskInstance.isRunning.


So if I modify the examples above:

const task = useStatefulTask({
  progress: 0
  progressPct: computed() => `${task.state.progress * 100}%`, // hm - no way to refer to the current instance here :(
  *perform() {
        while (this.progress < 100) {
        yield timeout(500);
        this.progress = this.progress + 0.01;
      }

     return 'done';
  }
});

This would kinda work. In order to show all the files in the template you could iterate over all running instances and show taskInstance.state.progress. There's a blocker with the computed though as there's no clear way how to access the current instance. Probably there's some workaround, but I fail to see it right now. But in this case the conversion to PCT could definitely be done in some view logic (component , pure function).

No need to create extra refs and reactive values.

you would need an additional setup function for each task

Having some kind of setup function in the task is an interesting idea as well and would for sure allow bigger flexibility. In this case the addFile logic could be moved there.


It seems there is a potential for taskInstance.state, perhaps even task.state. But overall the situation here is different to Ember. You can encapsulate by creating custom functions like useProgress and strictly use that. There's more flexibility in that regard. But still, taskInstance.state could still be used and could remove boilerplate code. After all task and taskInstance are already reactive objects, so why not use them more.

I'll look for potential usecase for encapsulated tasks in my work projects. When I find one, I'll try to experiment some more (create actual working prototype:)).

@lukaiser
Copy link

Cool! I am in favor for the idea of taskInstance.state and a setup function for it. I think task.state isn´t necessary with the Composition API. There only ever is one task. Something like this!?

const task = useStatefulTask(
    setup() {
        const progress = ref(0);
        const progressPct = computed(() => `${progress.value * 100}%`)
        *perform() {
            while (progress < 100) {
                yield timeout(500);
                progress = progress + 0.01;
            }
            return true;
        }
        return {perform, progress, progressPct}
    });

const totalProgress = computed(() => task.allRunningTasks.reduce((pv, cv) => return pv + cv.state.progress, 0));

I get confused by task and taskInstance too. I was wondering, if taskInstance is the wrong name? The Task Instance is in OOP the Instance of the task. Therefore, the object where you can call .perform() on. I was wondering if that confuses? Maybe the taskInstance should be called taskExecution, taskPerformance or taskRun or something like that.

@MartinMalinda
Copy link
Owner Author

MartinMalinda commented Sep 26, 2020

@lukaiser

I'd say the returned value should still end up on taskInstance.value even for statefulTask. So setting "side" state would have to be via this.progress = progress or perhaps even more explicitly via this.state.progress = progress. So this would refer to the current TaskInstance that is being run.

So taskInstance.state is something that's available always, while taskInstance.value is available after the instance has finished, afterreturn.

I still didn't have a usecase for this though. I can imagine some kind of multi-file upload would indeed be ideal (each while being one task instance).

I get confused by task and taskInstance too. I was wondering, if taskInstance is the wrong name? The Task Instance is in OOP the Instance of the task. Therefore, the object where you can call .perform() on. I was wondering if that confuses? Maybe the taskInstance should be called taskExecution, taskPerformance or taskRun or something like that.

Yeah it is a bit unfortunate. Especially here. In ember-concurrency it makes more sense because Ember itself is more OOP and ember-concurrency is also more object-oriented under the hood. But even there TaskInstance is not a true instance of a Task in the OOP sense.

Still I decided to keep the naming so that vue-concurrency si as close to ember-concurrency as possible. There's benefits to this consistency I think. But perhpaps there could be an upcoming major release that would introduce more fitting naming.
(But that's off-topic, a I'll create an issue for it:))

@MartinMalinda
Copy link
Owner Author

Soo, I happened to implement some upload with progress indicator and I can totally see a usecase for something like useStatefulTask. The danger is it can be quite powerful and ppl might start to use it instead of VueX, Pinia and so on.

I hacked a solution using normal tasks + refs on the side, but I'll try to refactor later with some new abstraction. Hopefully soon 🙏

@lukaiser
Copy link

Haha, you are right. But for everything that is powerful you must restrain yourself when to use it. The Composition API has the same pitfalls ;)

@MartinMalinda MartinMalinda added the enhancement New feature or request label Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants