Possibly rethink modifiers

[buschtoens]

Taking #6 (taskGroup) and #3 (maxConcurrency) into consideration, we might want to rethink how we apply modifiers. The current implementation of creating joint decorators for creating a task and applying a modifier has the advantage of being terse, but at the cost of reduced composability.

I have two ideas for different solutions. Possibly a mix of both might be best.

I'm intentionally taking this to ridiculous lengths here to demonstrate potential ugliness.

Chaining API

export default class ChainingExampleComponent extends Component {
  @task.restartable.maxConcurrency(5).on('click').cancelOn('keydown').evented.debug
  *someTask() {
    // ...
  }

  @taskGroup.restartable.maxConcurrency(5) choreGroup;

  // or maybe

  @task.group.restartable.maxConcurrency(5) choreGroup;

  @task.group('choreGroup')
  *choreTask() {
    // ...
  }
}

Composable / Stackable decorators API

export default class ComposableExampleComponent extends Component {
  @restartable
  @maxConcurrency(5)
  @on('click')
  @cancelOn('keydown')
  @evented
  @debug
  @task
  *someTask() {
    // ...
  }

  @maxConcurrency(5)
  @restartable
  @taskGroup
  choreGroup;

  @group('choreGroup')
  @task
  *choreTask() {
    // ...
  }
 
  // or

  @group('choreGroup')
  *choreTask() {
    // ...
  }

  // or maybe even

  @taskGroup('choreGroup')
  *choreTask() {
    // ...
  }
}

Considerations for the composable API:

• @on name clash with: Add @observes, @unobserves, @on, and @off decorators ember-decorators/ember-decorators#232. Would be cool, if one / our decorator could support both usages maybe.
• Stacking order: @task and @taskGroup (or @group) would always need to come last. Maybe we can make the order optional or even make @task implicit, so kinda like in the current implementation.

Excited for you thoughts! 😄

[gossi]

I would consider the stackable API to be more aligned what decorators (or annotations in java) are. Give how @argument works, this looks more aligned. Contrary to that, I like the shortness of the chainable API although it is likely to easily oversee one of the modifiers (and hell yeah, that's what we are gonna do 😝).

[buschtoens]

Yeah, I also lean more towards the stackable API for the very same reasons. It just feels more in line with the existing ecosystem.

We could clear the first concern (@on name clash) in two ways:

• rename to @performOn, which mirrors @cancelOn better any way
• get rid of .perform() as a whole and make tasks themselves executable, and then just use @on from @ember-decorators/object/evented

I don't know how feasible the last option is, but I think it'd be really sexy, not only for this case, but also in general, especially with ember-metal-es5-getters enabled.

class ExampleService extends Service {
  @on('init')
  @keepLatest
  @task
  *loadStuff() {
    // ...
  }

  async someMethod() {
    const value = await this.loadStuff();
    console.log(this.loadStuff.last.value === value);
  }
}

[dfreeman]

Yeah, I also lean more towards the stackable API for the very same reasons. It just feels more in line with the existing ecosystem.

👍

get rid of .perform() as a whole and make tasks themselves executable, and then just use @on from @ember-decorators/object/evented

I'd love that, but the last time that was investigated, I believe the conclusion was that it would make supporting all the nice derived state difficult. Ember.defineProperty(someFunction, 'key', computed(...)) appears to work fine, but I'm not sure there's any actual guarantee that that would continue to work. I also have a vague recollection that various JS engines' optimizations don't love it when you start throwing properties on functions?

[machty]

The current implementation of creating joint decorators for creating a task and applying a modifier has the advantage of being terse

I'm having trouble parsing this. Can you add an example of the "current implementation" with multiple decorators? (Sorry, in general I'm super rusty on decorators APIs and I don't personally use decorators or this repo)

[pzuraq]

Looking at annotation systems in other languages, and given there will likely be quite a few decorators working their way into JS and Ember in the near future, I’m not as sure about the stackable api. The main issue occurs when you start mixing two such systems together, for whatever reason.

We’ve had this happen in our Java frameworks in particular and it can get quite noisy very quickly. That said, Java also uses annotations for some pretty hacky things (e.g. @VisibleForTesting) so maybe this is a non-issue with decorators in JS

[buschtoens]

@machty

The current implementation of creating joint decorators for creating a task and applying a modifier has the advantage of being terse

I'm having trouble parsing this. Can you add an example of the "current implementation" with multiple decorators? (Sorry, in general I'm super rusty on decorators APIs and I don't personally use decorators or this repo)

Sure! 😊

So right now we have these decorators, which would be equal to the following proposed decorators:

@task

This will just stay @task.

@restartableTask

// stackable
@restartable
@task
*someTask() {}

// chainable
@task.restartable
*someTask() {}

@dropTask

// stackable
@drop
@task
*someTask() {}

// chainable
@task.drop
*someTask() {}

@keepLatestTask

// stackable
@keepLatest
@task
*someTask() {}

// chainable
@task.keepLatest
*someTask() {}

@enqueueTask

// stackable
@enqueue
@task
*someTask() {}

// chainable
@task.enqueue
*someTask() {}

Shortcomings of current API

#3 .maxConcurrency

maxConcurrency is missing and with the current approach of joining decorators, things would get awkward:

@restartableTaskWithMaxConcurrency(5) // 😵
*someTask() {}

Other missing functionality

• Add taskGroup decorator/s #6 taskGroup and .group
• .on / .cancelOn
• .evented
• .debug

[buschtoens]

@pzuraq

Looking at annotation systems in other languages, and given there will likely be quite a few decorators working their way into JS and Ember in the near future, I’m not as sure about the stackable api. The main issue occurs when you start mixing two such systems together, for whatever reason.

Are you talking about name clashes here? I share this concern. Or do you mean that, coming back to old code, you might not immediately see, which decorator belongs to which library?

We’ve had this happen in our Java frameworks in particular and it can get quite noisy very quickly. That said, Java also uses annotations for some pretty hacky things (e.g. @VisibleForTesting) so maybe this is a non-issue with decorators in JS

Do you worry about using to many decorators in general or "the size of the stack" with the stackable API?

[buschtoens]

  @restartable
  @maxConcurrency(5)
  @on('click')
  @cancelOn('keydown')
  @debug
  @task
  *someTask() {
    // ...
  }

This just looks absurd imo. But so does

  @task.restartable.maxConcurrency(5).on('click').cancelOn('keydown').debug
  *someTask() {
    // ...
  }

Wrapping lines might help a bit, because you have some sort of hierarchy through indentation:

  @task
    .restartable
    .maxConcurrency(5)
    .on('click')
    .cancelOn('keydown')
    .debug
  *someTask() {
    // ...
  }

Granted, this is a really extreme example.

However, thinking about all of your comments (thanks a lot, btw!), maybe a combination of the current approach and a one or both proposals might be best.

For example, we could keep the "base" modifiers as joint decorators. Meaning for task we'll have:

• @task
• @restartableTask
• @dropTask
• @keepLatestTask
• @enqueueTask

And the same for taskGroup:

• @taskGroup
• @restartableTaskGroup
• @dropTaskGroup
• @keepLatestTaskGroup
• @enqueueTaskGroup

A benefit of this is that we create no API churn for existing code bases already using this addon.

And, if I'm not missing anything, these are left:

• .maxConcurrency
• .group
• .on / .cancelOn
• .evented
• .debug

For those we can think about either exporting (some of) them as their own decorators or making (some of) them available as chainable API.

Some random thoughts:

Since tasks that are part of a .group cannot use modifiers, we could recycle the @taskGroup decorator, like

@taskGroup someTaskGroup;

@taskGroup('someTaskGroup')
*someTask() {}

.debug could / should be chainable API, since you'll only need it for development and having quick access to it, whithout having to import it, would be nice. The identifier debug is also so generic that it is likely to clash.

@keepLatestTask.debug
*someTask() {}

So now only

• .maxConcurrency
• .on / .cancelOn
• .evented

are left.

If we rename .on to @performOn, we could export it and @cancelOn as decorators as well.

So with these changes in mind, the above example could look like:

export default class ComposableExampleComponent extends Component {
  @performOn('click')
  @cancelOn('keydown')
  @evented
  @maxConcurrency(5)
  @restartableTask.debug
  *someTask() {
    // ...
  }

  @maxConcurrency(5)
  @restartableTaskGroup
  choreGroup;

  @taskGroup('choreGroup')
  *choreTask() {
    // ...
  }
}

[buschtoens]

@dfreeman

[...] it would make supporting all the nice derived state difficult. Ember.defineProperty(someFunction, 'key', computed(...)) appears to work fine, but I'm not sure there's any actual guarantee that that would continue to work.

Bummer. When I can find some time, I'll do a deep dive down the rabbit hole and try to understand how the logic currently works and what might change. :)

I also have a vague recollection that various JS engines' optimizations don't love it when you start throwing properties on functions?

Hm, could be. 🤔 But aren't properties on functions the equivalent to "static" properties (methods) on classes?

class FooClass {
  static staticMethod() {}
}

// is the same as

function FooClass() {}
FooClass.staticMethod = function() {};

[pzuraq]

Do you worry about using to many decorators in general or "the size of the stack" with the stackable API?

Having too many decorators in general. If we have two or three such stackable systems being applied to a single method, it could get confusing very quickly.

That said, unsure how much overlap there can be. For instance, the Argument decorators have almost zero overlap with tasks, so it would bw unlikely to see:

export default class ComposableExampleComponent extends Component {

  @argument
  @performOn('click')
  @cancelOn('keydown')
  @evented
  @maxConcurrency(5)
  @required
  @type(‘string’)
  doThing() { }
}

But you can see how it gets hard to parse, especially if users mix the decorators. The chaining method at least gives most modifiers context in complex systems like this.

Another option here could be an options hash. This was something that was probably less natural in the pre-decorators world, but works pretty well now:

@task({
  maxConcurrency: 5,
  performOn: ‘click’,
  cancelOn: ‘keydown’,
  debug: true
})
*someFunc() {}

[buschtoens]

That said, unsure how much overlap there can be. For instance, the Argument decorators have almost zero overlap with tasks [...}

I also cannot come up with a scenario where you'd wanna mix task decorators with other decorators. But yeah, you never know what the future holds. 🤷‍♂️

Another option here could be an options hash. This was something that was probably less natural in the pre-decorators world, but works pretty well now:

@task({
  maxConcurrency: 5,
  performOn: ‘click’,
  cancelOn: ‘keydown’,
  debug: true
})
*someFunc() {}

I can see myself using this. Looks decent. 😀

Do you think we should keep the joint decorators (@restartableTask, ...) still or put the modifier in the options hash as well?

@restartableTask({
  maxConcurrency: 5
})
*someFunc() {}

// vs

@task({
  modifier: 'restartable',
  maxConcurrency: 5
})
*someFunc() {}

// vs

@task({
  restartable: true,
  maxConcurrency: 5
})
*someFunc() {}

[pzuraq]

I like the idea of multiple decorators for the most common combinations (e.g. task, restartableTask). It feels like they're easier to parse. No reason we couldn't do both too, and let people decide. From a documentation POV, I could see benefits to either style.

[dfreeman]

Hm, could be. 🤔 But aren't properties on functions the equivalent to "static" properties (methods) on classes?

They are! I'm honestly not sure 😄 — I was essentially parrotting back something I read a very long time ago that may no longer be true or that I may have misunderstood in the first place.

[chriskrycho]

I rather like the idea of the hash of options, but with some high-level wrappers around them for common tasks.

It's roughly the same approach I took with the (currently undocumented and not ready for public consumption!) @olo/principled-forms field types.

The way it works there is: we have a Field (form field) which takes an options hash, and then we expose things like Number field and an Email field (and more in the future, presumably), and the argument hash to those is a subset of the Field hash – they do the work of setting up the arguments that are inherent in them and pass along the others. They're "just" helpers layered on top of the base functionality, but extremely convenient.

It works really nicely, and I think doing the same here with decorators is 💯.

[buschtoens]

Seeing that most seem to like the options hash with high-level wrappers, I think we can go ahead implementing it. One huge benefit of it being that we create zero API churn for existing users. ❤️

[buschtoens]

Thanks a lot everybody for sharing your ideas and thoughts!

I've implemented it and merged it to master. If you want, you can already test it as

yarn add -D machty/ember-concurrency-decorators

I'd like to wait with an official release until #10 is solved.