Conversation
c29d45e to
784e86d
Compare
twokul
left a comment
There was a problem hiding this comment.
I wonder if it makes sense to add this check within mergeTrees?
|
That would probably break lots of code. |
|
Oh our internal mergeTrees helper. Sorry I misread. That might work. |
|
@rwjblue @stefanpenner Does it make sense that our internal mergeTrees helper might return null? or is that a concern best left to the caller and always return a tree? |
f26e9a6 to
784e86d
Compare
lib/models/addon.js
Outdated
| annotation: `Addon#treeFor (${this.name} - ${treeType})`, | ||
| }); | ||
| let mergedTreesForType; | ||
| if (trees.length) { |
There was a problem hiding this comment.
can we be explicit ans use !== 0 here?
There was a problem hiding this comment.
actually thinking about it, should we use the fastpath for length === 1 too?
There was a problem hiding this comment.
our mergeTrees helper already has the length === 1 passthrough.
784e86d to
0c1ad34
Compare
|
@kellyselden have you tried moving that check into internal mergeTrees function? |
|
Yes. A lot of code wasn't expecting it, so I would like to make sure it is the right direction from others before fixing up all the code and tests. |
|
I think @rwjblue's utility aims to fix this: https://github.com/ember-cli/ember-cli/blob/master/lib/broccoli/merge-trees.js with the EmptyTree concept. |
|
@stefanpenner How so? That util always returns a tree, and in this case I want a falsy value. |
|
I could export |
lib/models/addon.js
Outdated
| annotation: `Addon#treeFor (${this.name} - ${treeType})`, | ||
| }); | ||
| let mergedTreesForType; | ||
| if (trees.length !== 0) { |
There was a problem hiding this comment.
The custom merge trees that we use (lib/broccoli/merge-trees.js) should already be taking care of this, can you check into why it isn't?
There was a problem hiding this comment.
But EMPTY_MERGE_TREE is truthy, is it not?
tests/unit/models/addon-test.js
Outdated
|
|
||
| extensionsForType() { | ||
| return ['js']; | ||
| return ['template']; |
There was a problem hiding this comment.
This seems odd, why are the test changes required?
There was a problem hiding this comment.
because Addon.treeFor can now return a falsy value where it didn't before. mergeTrees helper always returns a truthy tree.
0c1ad34 to
2ec95ae
Compare
|
@rwjblue updated with |
Without this, an empty array goes through mergeTrees and becomes a valid tree. With this change, it remains a falsy value and fails this check
ember-cli/lib/broccoli/ember-app.js
Line 618 in 203164d
This change is only a minor optimization now, but is more noticeable when paired with #7501.