make didInsertNode promise aware. fixes #57

[miguelcobain]

@jagthedrummer this version seems to pass the tests

Changes from your PR:

• this uses Promise.all on all the children's didInsertNode calls. Your version was basically not doing any waiting since you were calling forEach, which isn't promise aware.
• this moves this._didSetup = true; to the top of the setup function which is important for the timing of the registerChild which calls setup

Can you try this on your codebase?

I would love to add tests for this, but I didn't find a good way to test execution order with promises. 😞 I'm also considering if it makes sense to make willDestroyParent promise aware.

[jagthedrummer]

@miguelcobain, this is working great for me! When using my branch I had noticed that the children would fire didInsertParent twice, but I had thought it was due to some weirdness in my app code around async relationships and that the children were actually getting added to the DOM twice. Looks like it was probably due to the placement of this._didSetup = true;.

Anyway, this works great in my project. Seems like making willDestroyParent also be promise aware might not hurt.

Thanks for this!

[jagthedrummer]

Oh, I should also mention that I'm able to run tests locally now on this branch.

[jagthedrummer]

@miguelcobain, I took a stab at adding a test for callback ordering in #60 (which is pointed at the branch for this PR). If we care about children being called in the same order that they were added we have to await each of them individually instead of using Promise.all. For my purposes I don't particularly care that the children get called in a specific order, only that they all get called after the parent callback has completed. Do you think we should care about child callback ordering?

[miguelcobain]

@jagthedrummer I added some tests. QUnit's assert.step proved to be a much simpler way to test things than sinon. Unfortunately, there's a failing test for the async scenario. Still trying to understand if the problem is the promise-aware implementation or the tests themselves for async.

Failing test is didInsertParent hook runs in the correct order (async): top-level parent and two children-parents after if

[miguelcobain]

@jagthedrummer I refactored the project. It should be working now. Both didInsertParent and willDestroyParent are promise-aware, and it's all tested!

Can you test if this works on your codebase?

[jagthedrummer]

After switching to this branch I'm getting an error which seems to be preventing didInsertParent from firing in my child components.

Uncaught (in promise) TypeError: fn is not a function
    at Object.installModifier (did-insert.js:62:1)

And this is the bit of code that the first line of the stack trace points to:

It seems like ember/render-modifiers thinks that the function to call on did-insert isn't being passed in.

[miguelcobain]

This is weird. Are you 100% sure it comes from ember-composability-tools? The only place we're using the modifier is {{did-insert this.setup}}, and this.setup definitely exists.

[jagthedrummer]

OK, after studying all the changes in this PR I see that it's due to this PR introducing a breaking change.

I'm extending Root and Node and I had to change the template for my Root component.

I had to change this:

<div
  {{did-insert this.didInsertNode}}
  {{will-destroy this.willDestroyNode}}
>
  ...
</div>

To this:

<div
  {{did-insert this.setup}}
>
  ...
</div>

Fwiw, this is related to the fact that I'm still extremely unclear on whether the intended use for ect is to extend Root & Node or to just use them directly. The README says to extend them, but doesn't provide a complete example. And the info in the README seems to contradict the advice you gave in this comment. Maybe both approaches are valid depending on what you're trying to do? It would be super helpful to get some clarification on that point.

But confusion aside, once I updated my project to account for the breaking change everything seems to be working fine.

[miguelcobain]

@jagthedrummer I see. I think I should fix that story. The problem with extending Root or Node is that the template gets overriden, which is an important part of how the whole system works.

I think the best way at the moment is to use Root and Node directly in your own components. Definitely the "safest" way. The only downside I see is having access to this.children. We could yield the children, but you would have to somehow propagate that to your component's js.

If you don't need this.children, then using the Root and Node components directly in your own components is the best way for sure.

[jagthedrummer]

Is there anything I can do to help get this landed and released?

[jagthedrummer]

Yay! Thanks so much, @miguelcobain. Any chance you could publish a new release?

[miguelcobain]

@jagthedrummer version 1.3.0 was published

[jagthedrummer]

Thanks a bunch, @miguelcobain! And thanks again for this excellent addon. I owe you a few beverages if there's ever the opportunity. Cheers!