Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ember 2.10.0 helper recompute bug #14774

Closed
ssured opened this issue Jan 3, 2017 · 13 comments · Fixed by #18110
Closed

Ember 2.10.0 helper recompute bug #14774

ssured opened this issue Jan 3, 2017 · 13 comments · Fixed by #18110

Comments

@ssured
Copy link
Contributor

ssured commented Jan 3, 2017

Calling recompute on a helper does not recompute in 2.10.0

Twiddle: https://ember-twiddle.com/3afc1bda1c824f4ed141b666e5521320?openFiles=helpers.my-helper.js%2C

When you change the dependency to 2.9 this code works, for 2.10 it does not work. It might be related to Glimmer update?

@rwjblue
Copy link
Member

rwjblue commented Jan 3, 2017

Definitely seems funky, I agree. The work around for now is to ensure the recompute is done within a run-loop (which fixes the twiddle):

export default Ember.Helper.extend({
  count: 0,
  compute() {
    if (!this.timer) {
      this.timer = window.setInterval(() => {
        Ember.run.join(() => {
          this.incrementProperty('count');
          console.log('call recompute', this.get('count'));
      	  this.recompute();
        });
      }, 500);
    }    
    return "count = " + this.get('count');
  }
});

Off the top of my head, I am not sure why a run-loop is required for it to function, but wrapping sets/rerenders in a run.join like this is almost certainly better for testing (would prevent an auto-run assertion in tests) so it is a reasonable solution.

@ssured
Copy link
Contributor Author

ssured commented Jan 5, 2017

My fault, I did not think of the runloop. Would a solution be to wrap

this[RECOMPUTE_TAG].dirty();
in a run.join to prevent this for future users?

@ssured ssured closed this as completed Jan 5, 2017
@youroff
Copy link

youroff commented Aug 6, 2017

Why is this closed? Seems like an actual bug or what's the rationale behind the need of manually wrapping recompute call in run.join?

@pixelhandler
Copy link
Contributor

@locks @rwjblue @ssured @youroff is this still an issue, perhaps we should close or create a new reproduction of this, what do you think?

@boris-petrov
Copy link
Contributor

boris-petrov commented Oct 5, 2018

@pixelhandler - we just hit the same issue in Ember 3.4.5. So it is still valid. We had to wrap the call to recompute in a run. I created another issue a few days ago which is very similar to this problem that we face here. I.e. the code we had was:

export default Helper.extend
  compute: ([a, b]) ->
    pendingResult = @get('_pendingResult')
    if pendingResult?
      @set('_pendingResult', undefined)
      return pendingResult

    result = a.someMethodWithNativeAwaitsInside(b)

    result.then (result) =>
      @set('_pendingResult', result)
      @recompute()

    false

And this "blocked" when recompute was called - i.e. it didn't call compute again. And this started appearing only after we changed our code to use native promises instead of RSVP promises (or most of it - because of the other issue I mentioned). So there is some inconsistency between RSVP promises and native promises (under the latest Chrome) - and because of it, stuff breaks.

Any ideas?

P.S. I tried creating a reproduction but couldn't do it for now. But I'm pretty sure that fixing the other issue will also fix this.

@rwjblue
Copy link
Member

rwjblue commented Oct 11, 2018

The solution here is to ensure we run.join inside the recompute code (as @ssured mentioned in a prior comment).

@boris-petrov
Copy link
Contributor

boris-petrov commented Oct 11, 2018

@rwjblue - yes, this is also how we worked around the issue, but the original problem is still there. Please check the reproduction repo I've given in the other GitHub issue.

@rwjblue
Copy link
Member

rwjblue commented Oct 11, 2018

I don't understand what you mean @boris-petrov. I'm saying that Ember.Helper's implementation of recompute should be using run.join not that your user-land helper code should...

@boris-petrov
Copy link
Contributor

boris-petrov commented Oct 11, 2018 via email

alexlafroscia added a commit to alexlafroscia/ember-stream-helper that referenced this issue Jan 26, 2019
I ran into this bug

emberjs/ember.js#14774

so I followed the suggestion and wrapped the `recompute` in a runloop
@xg-wang
Copy link
Contributor

xg-wang commented May 5, 2019

This issue can be worked around by creating a new runloop as explained in adopted-ember-addons/ember-router-helpers#126.
@rwjblue should recompute's internal impl be using runloop or just let userland code doing the run.join workaround?

@buschtoens
Copy link
Contributor

I also just ran into this, because ember-on-modifier does not create a new runloop. I'm not really sure, whether we should consider this a bug in the framework or expected behavior.

@rwjblue
Copy link
Member

rwjblue commented Jun 17, 2019

Definitely a bug in the framework. We need to fix 😢.

@buschtoens
Copy link
Contributor

I'm saying that Ember.Helper's implementation of recompute should be using run.join not that your user-land helper code should...

Easy PR right there! I'll take it tomorrow, if nobody else signs up. 😏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants