fix #480, use scheduleOnce instead of run #481
Conversation
…as we receive a value change message
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
1 similar comment
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
|
CLAs look good, thanks! |
1 similar comment
|
CLAs look good, thanks! |
|
@olivierchatry looks like this PR has introduced regression with acceptance tests, #484. |
|
Yep, seems like it, I'm not sure what to do there. And also, seems like emberfire is slowly dying :/ |
|
Not sure if it is of any interest but : http://stackoverflow.com/questions/28746145/what-is-the-proper-way-in-ember-to-use-scheduleonce-that-is-test-friendly |
|
Wrapping in |
|
Hopefully it should, but I did not try.
…On Mon, Apr 10, 2017 at 2:52 PM, Tomasz Subik ***@***.***> wrote:
Wrapping in Ember.run could be a way, as normally if there is no open run
loop it would create one, but does it resolve the original problem with
Ember data. I would have to reproduce this problem.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#481 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABNXG0kw7GZzs-PJRrWbxzLqiQmYaySKks5ruiYHgaJpZM4L1DZ9>
.
|
|
@olivierchatry do you have some repo to reproduce the original problem with infinite loop? I tried to reproduce that on my side project but without success. |
|
Nop, it happens multiple time on my software ( and made sense at this point
), but that was about 2 months ago. Is the Ember.run fix does not work for
you ?
…On Wed, Apr 12, 2017 at 1:36 PM, Tomasz Subik ***@***.***> wrote:
@olivierchatry <https://github.com/olivierchatry> do you have some repo
to reproduce the original problem with infinite loop? I tried to reproduce
that on my side project but without success.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#481 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABNXGzDM91yH_2Dx39ZQT2F2S9M3wTf1ks5rvLdNgaJpZM4L1DZ9>
.
|
|
@olivierchatry I'm not an expert in run loop. I had to refresh my memory about it as I'm no longer working in Ember full-time :/, but I think just one |
|
I will remove the fix and try to reproduce the issue so that I can have a
clear answer. Should have done a unit test with the PR :(.
…On Thu, Apr 13, 2017 at 1:19 PM, Tomasz Subik ***@***.***> wrote:
@olivierchatry <https://github.com/olivierchatry> I'm not an expert in
run loop. I had to refresh my memory about it as I'm no longer working in
Ember full-time :/, but I think just one scheduleOnce action does not
make sense with Ember.run wrap. It should work the same way as just
wrapping resolve, reject in simple Ember.run, which means simple revert
of this PR will suffice. That's why I was asking about the repo to
reproduce the original problem because I could be wrong :P. And yes, either
wrap or revert to previous code solves problems with acceptance testing.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#481 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABNXGwV29t-QVGoFnJQK4kSovQr-Meabks5rvgSpgaJpZM4L1DZ9>
.
|
|
Thanks @olivierchatry 👍 |
|
Yeah I was not able to reproduce the bug, but I did not try that hard either. Anyway, we can probably revert this PR if it a bother for a lot of people ( even if I think it is better to not fire up immediately the change ) . |
|
@jamesdaniels anyway you can do that ? and also maybe release a new version ? |
|
I've added PR for that :) |
|
@olivierchatry done. |
Description
Use scheduleOnce instead of firing as soon as we receive a value change message. Had to set ember-data to 2.10 for test to pass. This avoid entering in a long loop that make ember think we are infinite looping.