Fix run order of didInsertParent for children of already-set-up parents
#11
Conversation
| let parentSpy = this.parentSpy = this.spy(); | ||
| let childSpy = this.childSpy = this.spy(); | ||
| let childParentSpy = this.childParentSpy = this.spy(); | ||
| test('top-level parent with children-parents after if', function(assert) { |
There was a problem hiding this comment.
Why did you change existing tests rather than adding a new one?
There was a problem hiding this comment.
I actually started out with it in a separate test. I moved it here because it wasn't testing a new thing, just clarifying of how the run order should work for parents that were already set up.
I'm happy to split it back out again if you prefer.
There was a problem hiding this comment.
@miguelcobain Ping. Would you like me to split this out into a separate test?
There was a problem hiding this comment.
Sorry for the delayed response, @reidab. Yes please, that would help a lot for me to understand what is going on here!
addon/mixins/child.js
Outdated
| @@ -55,11 +55,18 @@ export default Mixin.create({ | |||
| } | |||
| }, | |||
|
|
|||
| notifyParentOfInsert: on('didInsertElement', function() { | |||
There was a problem hiding this comment.
We should use didInsertParent instead of on('didInsertParent',. It makes the calling order more predictable and provides a better opportunity for subclasses to override the method.
There was a problem hiding this comment.
I went with on('didInsertElement'… explicitly to avoid subclasses being able to override this and break the chain. I'll swap it.
|
I'd completely forgotten about this PR until I went to update another library I had that depended on it. Apologies! I've updated the PR to split out the new tests into their own test case and reverted back the changes to existing tests. |
|
Ping. I've been delaying a library depending on this fix for many months now. Is there anything else I can do to move it forward? |
reidab
force-pushed
the
didInsertParent-for-already-setup-parents
branch
from
24d3383
to
0b8e1b0
Compare
|
Just rebased the branch on the current master resolved conflicts. |
|
@miguelcobain I just made PR #19 that will fix the Travis failures seen in this PR. |
|
The description in @Duder-onomy's PR #19 said that it fixed this, so this got auto-closed, but it hasn't actually been merged. 😢 |
|
@miguelcobain This is still an issue, My PR accidentally closed this.. All my PR did was fix TravisCi. |
|
@miguelcobain Can this be merged? |
Sharing a single test spy between the two child-components can confuse order-dependent tests referring to the order of events on a single component. Since the second child-component isn't necessary for these tests, I'm removing it rather than giving each child-component its own spy.
…y-set-up parents Fixes miguelcobain#10
Duder-onomy
force-pushed
the
didInsertParent-for-already-setup-parents
branch
from
0b8e1b0
to
c0bb308
Compare
|
@miguelcobain This is still an issue, i'm going to try and explain the issue the best I can. When the change was made to register children on |
|
Ping, just for visibility. |
|
The new architecture of ember-composability-tools is a lot different. In any case, I believe this is fixed, according to my tests. Feel free to reopen if this issue is found. |
Fixes #10