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

Introduce ember-concurrency with examples #977

Merged

Conversation

sukima
Copy link
Contributor

@sukima sukima commented Mar 7, 2017

Closes #969

This PR adds the ember-concurrency addon and includes two examples of refactors.

  • Example one shows how to use e-c in a route.
  • Example two shows how to simplify complex promise logic with e-c.

I wrote more notes in the commit messages.

@sukima
Copy link
Contributor Author

sukima commented Mar 7, 2017

I can not get the tests to fail locally (in browser or in phantom/CI). Is this a know bug?

Oddly the failed assertion is failing with the same predicate that the wait helper polls on the line before it. It seems odd that the element is there andThen the very next statement it is gone:

    andThen(() => {
      waitToDisappear('.modal-dialog');
      // Wait with polling till this element exists (length > 0)
      waitToAppear('a.secondary-diagnosis:contains(Tennis Elbow)');
    });
    andThen(() =>{
      // It dies here because it can not find the element (length === 0)
      assert.equal(find('a.secondary-diagnosis:contains(Tennis Elbow)').length, 1, 'New secondary diagnosis appears');
      click('a:contains(Add Operative Plan)');
      waitToAppear('span.secondary-diagnosis:contains(Tennis Elbow)');
    });

WTF am I missing?!

@sukima
Copy link
Contributor Author

sukima commented Mar 7, 2017

1..907
# tests 907
# pass  907
# skip  0
# fail  0

# ok

Both on master and on this branch. Beyond confused. I swear Travis hates me.

@jkleinsc
Copy link
Member

jkleinsc commented Mar 7, 2017

@sukima welcome to my pain with Travis! Yeah, I'm not sure what/why it does that. Usually I just restart Travis and it will pass. I'll do that with this PR and see if it passes.

@sukima
Copy link
Contributor Author

sukima commented Mar 7, 2017

@jkleinsc Do you think it might be related to #976?

@jkleinsc
Copy link
Member

jkleinsc commented Mar 7, 2017

@sukima it is a different issue because I can get it to reproduce at various times.

Copy link
Member

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sukima I really like how this cleans up the code. Thanks for the PR! I'm in favor of merging this in, as long as you can make the change suggested below.

});
}
}.property()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have been trying to move HospitalRun away from using property() to define computed properties
Can we change this to follow this: https://github.com/DockYard/styleguides/blob/master/engineering/ember.md#dont-use-embers-prototype-extensions

Basically change

defaultAddressOption: function() {
...
}.property()

to

defaultAddressOption: computed(function() {
...
})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely! I prefer DockYard's style better. I only kept this because I thought it was the style choice based on looking at the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. And rebased!

@sukima sukima force-pushed the feature/introduce-ember-concurrency branch 2 times, most recently from 5b37175 to 57e63fb Compare March 8, 2017 22:41
This is the example I used in issue HospitalRun#969. It is illustrates how to use
ember-concurrency tasks in routes.
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.
@sukima sukima force-pushed the feature/introduce-ember-concurrency branch from 57e63fb to b71ed05 Compare March 8, 2017 22:41
@sukima
Copy link
Contributor Author

sukima commented Mar 13, 2017

@jkleinsc Bump. changes complete. Ready for review.

@jkleinsc
Copy link
Member

@sukima Looks good to me. I'll merge it in today.

@jkleinsc jkleinsc merged commit 78a2023 into HospitalRun:master Mar 16, 2017
@sukima sukima deleted the feature/introduce-ember-concurrency branch March 17, 2017 02:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants