machty / ember-concurrency

ember-concurrency is an Ember Addon that enables you to write concise, worry-free, cancelable, restartable, asynchronous tasks.
http://ember-concurrency.com
MIT License
690 stars 157 forks source link

Avoid using incrementProperty to workaround duplicate auto-tracking assertion in Ember 3.15 #338

Closed maxfierke closed 4 years ago

maxfierke commented 4 years ago

(Hopefully) Fixes #337 🤞

Draft until there's a reproducible test case

Turbo87 commented 4 years ago

@maxfierke yep, I ran it through our CI and it indeed seems to fix the issue. This is slightly surprising to me though, since I would expect incrementProperty() to have the exact same implementation as what you replaced it with 😅

pichfl commented 4 years ago

This is the implementation: https://github.com/emberjs/ember.js/blob/d4dc4b4cc587cbacdfb1958fbd03eb9770efe9ab/packages/%40ember/-internals/runtime/lib/mixins/observable.js#L439-L445

My wild guess is, that this is caused by some of the magic behind the nested getters in tracked properties (since nested props are incremented here) and setting them directly might work around that.

In other words: Works in my project as well.

patocallaghan commented 4 years ago

This fix worked for us too 👍

maxfierke commented 4 years ago

@Turbo87 @pichfl @patocallaghan would any of you be able to provide a small test case or steps for reproduction for this issue? I'd like to have a test in place before merging this, and I'm not sure how to reproduce this, as the test suite is passing against 3.15+

Turbo87 commented 4 years ago

I finally was able to come up with a small repro:

    this.owner.register(
      'component:e-c-test',
      Ember.Component.extend({
        layout: hbs`<input>`,

        focusIn() {
          this.exampleTask.perform();
        },

        exampleTask: task(function*() {
          yield timeout(100);
        }),
      })
    );

    await render(hbs`<ECTest {{autofocus}} />`);

with {{autofocus}} coming from https://github.com/qonto/ember-autofocus-modifier, but I suspect with a did-insert it would probably also trigger.

maxfierke commented 4 years ago

Spent a little time playing around with did-insert before finding that auto-tracking is disabled for the @ember/render-modifiers, so they didn't trigger the issue (see https://github.com/emberjs/ember-render-modifiers/commit/2a8d1a10fb4c7e80e01ae86bb1a012e72632994a). Just went with the {{autofocus}} example and was able to reproduce in a test. Thanks a bunch @Turbo87! I'll get this merged and released sometime in the next day or so.

--

Narrator: It took longer than a day