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

[Glimmer 2] Assertion Failed: You modified [property] twice in a single render #14013

Closed
GavinJoyce opened this issue Aug 3, 2016 · 4 comments

Comments

@GavinJoyce
Copy link
Member

GavinJoyce commented Aug 3, 2016

I'm not certain that this is a supported case, but this is an issue that I've come across a number of times in Intercom's app.

export default Ember.Component.extend({
  attributeBindings: ['style'],
  count: 0,
  expanded: false,

  style: Ember.computed('expanded', function() {
    this.incrementProperty('count');
    if(this.get('expanded')) {
      return `background-color: lightgreen`.htmlSafe();
    } else {
      return `background-color: yellow`.htmlSafe();
    }
  }),

  actions: {
    toggle() {
      this.toggleProperty('expanded');
    }
  }
});

It works if I remove the this.incrementProperty('count');.

Twiddles:

Canary (:green_apple:): https://ember-twiddle.com/c78fcbe4267c069f3898c28e3562962f?openFiles=components.my-component.js%2Ctemplates.components.my-component.hbs

Alpha (:red_circle:): https://ember-twiddle.com/0b95e3a8dcacabfbe3fe67c0ed240ec9?openFiles=components.my-component.js%2Ctemplates.components.my-component.hbs
which raises Uncaught Error: Assertion Failed: You modified count twice in a single render. This was unreliable and slow in Ember 1.x and is no longer supported.

Alpha with {{style}} in template (:green_apple:): https://ember-twiddle.com/1f5550724172dfb5cb67870dbb744648?openFiles=components.my-component.js%2Ctemplates.components.my-component.hbs
which works and leads me to suspect that this is a bug in Glimmer.

This works in both Canary and Alpha:

export default Ember.Component.extend({
  attributeBindings: ['style'],
  count: 0,
  expanded: false,

  style: Em.computed('expanded', function() {
    if(this.get('expanded')) {
      return `background-color: lightgreen`.htmlSafe();
    } else {
      return `background-color: yellow`.htmlSafe();
    }
  }),

  actions: {
    toggle() {
      this.toggleProperty('expanded');
      this.incrementProperty('count');
    }
  }
});

LMK if this is a bug and I'll add a failing test case

GavinJoyce added a commit to GavinJoyce/glammer that referenced this issue Aug 3, 2016
@chadhietala
Copy link
Contributor

chadhietala commented Aug 3, 2016

Yea I think this is going to create unpredictable behavior sense you are essentially setting in a CP getter which is something we would like to avoid. Moving the incrementProperty to init and then also incrementProperty in the action it's self gets you the desired result.

https://ember-twiddle.com/6c0b3a6424186394b433290afcc0ea18?openFiles=components.my-component.js%2Ctemplates.components.my-component.hbs

I think we should lint for these types of patterns. /cc @rwjblue @chancancode @krisselden

@chancancode
Copy link
Member

Is count referenced anywhere else in a template?

@chancancode
Copy link
Member

This is #13948 by the way. However, in this case it could be a bug if count is never referenced in a template

@GavinJoyce
Copy link
Member Author

GavinJoyce commented Aug 3, 2016

Thanks. Yes, {{count}} is referenced in the template. As you suspected, there is no error when I remove it from the template.

The thing that threw me was that simply adding {{style}} to the template results in no exception (twiddle). Is there some reason why we shouldn't be raising the same You modified count twice in a single render exception in this case?

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

No branches or pull requests

3 participants