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

Don't crash if a child has no parent #14

Closed lolmaus closed 6 years ago

lolmaus commented 6 years ago

I'm revising my ember-element-query addon. ember-composability-tools is gonna help me resolve an incompatibility with Glimmer 2 and optimize performance. :heart_eyes:

In ember-element-query the same component serves both as parent and child. Instances of this component form a tree-like structure.

The root node of this tree is a component without a parent.

Maintaining a separate version of the component, with the only difference being not including the child mixin, is a hu-u-uge bummer. It's just not reasonable.

This PR resolves the problem. Instead of crashing, a parentless child node simply does not register with a parent.

I've tried it locally and it works great. I also manually tested leaving the route and going back, everything works as expected.

miguelcobain commented 6 years ago

Is using shouldRegister: false not an option in your addon? Or making that root node to not have ChildMixin (because it is not a child after all)?

Nevertheless, I tend to agree with this PR. I just can't remember why that code is even there. Maybe I wanted to let people know if they were using a child outside of a parent context to not have "silent failures".

Btw, to pass tests, please remove the assert destructure assignment.

lolmaus commented 6 years ago

Is using shouldRegister: false not an option in your addon? Or making that root node to not have ChildMixin (because it is not a child after all)?

When you use ember-element-query in a component, you can't be sure where this component is used. In one route it'll be parentless, but another route may also be using ember-element-query, so the component ends up having a parent.

You can configure it by hand by passing a shouldRegister: false property, but that's extremely tedious.

Also, imagine that one day you apply ember-element-query to a route template, and its components behave incorrectly because they have been marked as shouldRegister: false. And you have to go through all your templates and update the shouldRegister option, wondering if you missed any.

All of that is completely unnecessary.

If I were implementing ember-composability-tools I'd go even further: use a single mixin for both parent and child. Does splitting it into two give a performance benefit?

Maybe I wanted to let people know if they were using a child outside of a parent context to not have "silent failures".

But is it even a failure? The hook will still work correctly in relation to its children.

Btw, to pass tests, please remove the assert destructure assignment

Thx, I edited via GitHub UI which doesn't have linting tools. :trollface: I've force-pushed a fix.

lolmaus commented 6 years ago

@miguelcobain The failing build seems to be unrelated to the PR, can you please have a look?