search

miguelcobain / ember-composability-tools

ember-composability-tools - Helpers for building a somewhat different kind of components.
MIT License
39 stars 18 forks source link

fix init to run superwrapper only once #8

Closed canufeel closed 7 years ago

canufeel commented 7 years ago

Previously it would run twice which would completely break paper-button with icons for ember-paper > v1.0.0-alpha.10 by causing infinite loop in ember-hammertime (adding it style attributeBindings twice and starting to rely on just added one https://github.com/html-next/ember-hammertime/blob/master/addon/mixins/touch-action.js#L85 as the otherStyleKey which leads to infinite loop)

miguelcobain commented 7 years ago

@canufeel I tried to reproduce this using a more similar approach to the existing tests: https://github.com/miguelcobain/ember-composability-tools/commit/03b99caadba35d52bb1bcbe25949daea9bd43df4

The test passes. This problem seems very strange, I don't see how calling init twice can possibly happen. Maybe you can enlighten me.

canufeel commented 7 years ago

Basically as i understand the calls to this._super(...arguments) work by wrapping a super function - https://github.com/emberjs/ember.js/blob/c329f95c1b07b4f70c74b075803dac8f8ba40e23/packages/ember-utils/lib/super.js#L30. When Ember executes init() hook it first uses this wrap method on it setting this._super to be equal to init of Ember.Component itself. So until the extended component's init has not finished execution this._super would still point to Ember.Component's init method. In our case in initChild or initParent we would still invoke Ember.Component init method which is done twice. That is probably fine if we don't have any additional logic in between Ember.Component and our mixin in inheritance chain but when we do and that logic relies on single execution it might break things.

miguelcobain commented 7 years ago

@canufeel I see. I thought we would be invoking initChild/initParent in those cases. It's not trivial to test it the way I wanted to.

canufeel commented 7 years ago

Probably we should use - https://github.com/emberjs/ember.js/blob/676fa7e774ada3951d37478ff22feb30f85604bb/packages/ember-metal/lib/mixin.js#L590

That apply would setup _super wrapper for function called

But not really sure about that

miguelcobain commented 7 years ago

I merged this.

  1. the test is testing correctly the bug
  2. no other tests broke

It should be safe. Let me know if you find a more elegant way to test this (using sinon and render).

canufeel commented 7 years ago

Ok, sure! Thanks!