opened
chancancode
username_0
Had I known that this is a thing, I would probably have written the types a bit differently, but this should work and is non-breaking.

I didn't make `EncapsulatedTaskInstance` public because I had this lingering suspicious that we'll need to add a second generic parameter to `TaskInstance` for the args. I don't want to lockdown that possibility, or deal with strangely mismatched ordering of parameters between `TaskInstance` vs `EncapsulatedTaskInstance`, so I kept that private.

For the time being, the public way to type it (as seen in the tests) is `TaskInstance<T> & { ... }`, which I think is plenty acceptable.

Got to update e-c-async and e-c-ts to account for this, then update the Octane tests.

cc @username_1
Allow accessing encapsulated task state
issue
created
andreyfel
username_1
Hi @username_0! Thank you for this PR! It seems that it is covering my use case! I've tested against that branch and it works!

The only thing I'm missing now is how to type `this` within the `perform` function of the encapsulated task. If I want to set a property on task instance inside perform what type does it have? EncapsulatedTaskInstance?
comment
created
chancancode
username_0
I don't think we should block on making `EncapsulatedTaskInstance` public. It is very rare that you would have to type it (please show some examples – most cases I know of are either implicit or can be inferred, and I think it's rare that you would want to use this in a parameter or return position).

I do think people will copy the type alias, and I think that's not ideal but not so bad. If we a public one, it will be compatible with and won't break these existing usages.

I do think we should work towards making it public, we just have to address the other blocker first–  whether we need to add the second generic to `TaskInstance` – and that's far from a hypothetical/theoretical issue. I don't think this PR is a good place to address that but we can also come to a decision on that fairy soon and fix both.

In the meantime, I think this adds enough values to those who need the feature and it's not obvious to me that they would be inconvenienced by this during normal usage.
comment
created
chancancode
username_0
Why did you have to type it? There are tests that confirms the `this` inference works correctly: https://github.com/username_0/ember-concurrency/blob/encapsulated-task-v2/tests/types/ember-concurrency-test.ts#L2347-L2348

Is that not working for you for some reason?
comment
created
andreyfel
username_1
@username_0, yes, it works in a basic case. But I have a little bit more complex case.
The idea is to have a task which exposes a property isRunningLong which should turn true in
case if the task takes longer than `loadingTimeout`. I'm using an encapsulated task with an inner task for it:

```
export interface LongRunningTaskDescriptor<T, Args extends any[]> extends EncapsulatedTaskDescriptor<T, Args> {
  isRunningLong: boolean,
  timeoutTask: TaskProperty<void, []>,
}

export function longRunningTask<T, Args extends any[]>(fn: TaskFunction<T, Args>, loadingTimeout = 300): LongRunningTaskDescriptor<T, Args> {
  return {
    isRunningLong: false,

    timeoutTask: task(function * (this: LongRunningTaskDescriptor<T, Args>) {
      yield timeout(loadingTimeout);
      set(this, 'isRunningLong', true);
    }).drop(),

    * perform(...args): TaskGenerator<T> {
      try {
        this.timeoutTask.perform();
        return yield * fn.apply(this.context, args);
      } finally {
        this.timeoutTask.cancelAll();
        set(this, 'isRunningLong', false);
      }
    },
  };
}
```

It is used like this with new e-c and e-c-d:
```
@task({ restartable: true })
fetchSmth = longRunningTask(function * () {
    yield fetchSmth();
}, 600)
```

So, I have to specify type of `this` for the `timeoutTask`. And inside the `perform` function this.timeoutTask.perform doesn't work. And `taskFor` doesn't work here.
Also typescript says that this.context doesn't exist. I'm not sure if it is a type issue or context is not a public filed.
comment
created
maxfierke
username_2
`context` is sort of "intimate", undocumented API. It's useful for implementing extensions onto e-c, so perhaps its worth making public but its not something we'd expect people to use often (though with encapsulated tasks, it might make more sense.)

I think a composition like that to create an encapsulated task is a bit of an advanced use-case, so if it's working in the basic case, it may be worth deferring for later unless addressing it is fairly straightforward.
comment