Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

Concider ember-concurrency addon #969

Closed
sukima opened this issue Mar 2, 2017 · 2 comments
Closed

Concider ember-concurrency addon #969

sukima opened this issue Mar 2, 2017 · 2 comments

Comments

@sukima
Copy link
Contributor

sukima commented Mar 2, 2017

I am planning on joining Slack but also thought it would help documenting this question here for discussion.

Has this project considered ember-concurrency?

An Ember add-on that uses ES6 generator functions to reduce the boilerplate around concurrency while adding structure to complex asynchronous code. [LinkedIn's] experience has been overwhelmingly positive.

It would make code like this:

model() {
  return new Ember.RSVP.Promise((resolve, reject) => {
    this.get('store').find('option', 'address_options').then((addressOptions) => {
      resolve(addressOptions);
    }, (err) => {
      if (err instanceof UnauthorizedError) {
        reject(err);
      } else {
        let store = this.get('store');
        let newConfig = store.push(store.normalize('option', {
          id: 'address_options',
          value: {
            address1Label: this.get('i18n').t('admin.address.addressLabel'),
            address1Include: true
          }
        }));
        resolve(newConfig);
      }
    });
  });
}

Look like this:

model() {
  return this.get('addressTask').perform();
},

addressTask: task(function * () {
  const store = this.get('store');
  const i18n = this.get('i18n');

  try {
    return yield store.find('option', 'address_options');
  } catch (err) {
    if (err instanceof UnauthorizedError) {
      throw err;
    }
    return store.push(store.normalize('option', {
      id: 'address_options',
      value: {
        address1Label: i18n.t('admin.address.addressLabel'),
        address1Include: true
      }
    }));
  }
})
.keepLatest()
.cancelOn('deactivate')

It can exists with the current code but allow refactors, clean up, and expand new features. I mention this because I and many others in the community have found this invaluable and door opening.

@jkleinsc
Copy link
Member

jkleinsc commented Mar 3, 2017

@sukima I've heard of ember-concurrency but haven't had time to look into to it. I am in favor of simplifying things and I see how this could help. If you (or others) want to submit a PR introducing ember-concurrency (on a small subset of code), that would be encouraged and would probably be a good next step in demonstrating ember-concurrency.

@sukima
Copy link
Contributor Author

sukima commented Mar 3, 2017

I would be happy to make a PR with the above initial conversion. Any troublesome async areas you would want tackled (I am new to the code base) let me know I can look at them too. I kinda like refactoring like this.

sukima added a commit to sukima/hospitalrun-frontend that referenced this issue Mar 7, 2017
This is the example I used in issue HospitalRun#969. It is illustrates how to use
ember-concurrency tasks in routes.
sukima added a commit to sukima/hospitalrun-frontend that referenced this issue Mar 8, 2017
This is the example I used in issue HospitalRun#969. It is illustrates how to use
ember-concurrency tasks in routes.
sukima added a commit to sukima/hospitalrun-frontend that referenced this issue Mar 8, 2017
This is the example I used in issue HospitalRun#969. It is illustrates how to use
ember-concurrency tasks in routes.
jkleinsc pushed a commit that referenced this issue Mar 16, 2017
* Add ember-concurrency addon version 0.7.19

* Refactor admin/address/route to use e-c

This is the example I used in issue #969. It is illustrates how to use
ember-concurrency tasks in routes.

* Refactor patients/delete/controller to use e-c

Example of using ember-concurrency tasks to manage a complex promise
chain of events. I picked this one mainly because I wanted to illustrate
the simplicity of using e-c tasks to manage what was otherwise a very
complex promise chain.

I tried to preserve some of the concurrency described in the previous
promise based code. However, after some analysis and discussion on the
Ember Slack channels difference between preserving the concurrency and
just running the resolutions serially are likely very small. In this
case the use of `all()` could likely be removed without a significant
impact on performance. I mention this later optimisation as a way to
make the code even easier to grok. I'll leave the debate to further
discussion.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants