Unique initializers #89
Conversation
| @@ -89,7 +89,11 @@ function mergeComposable(dstDescriptor, srcComposable) { | |||
| combineProperty('deepConfiguration', merge); | |||
| if (Array.isArray(srcDescriptor.initializers)) { | |||
| if (!Array.isArray(dstDescriptor.initializers)) dstDescriptor.initializers = []; | |||
| dstDescriptor.initializers.push.apply(dstDescriptor.initializers, srcDescriptor.initializers.filter(isFunction)); | |||
| srcDescriptor.initializers.forEach(i => { | |||
There was a problem hiding this comment.
Would be using reduce better? Something like this without side effects and in my opinion somewhat easier to reason about.
dstDescriptor.initializers = srcDescriptor.initializers.reduce((result, init) => {
if (isFunction(init) && !result.includes(init)) {
result.push(init);
}
return result;
}, Array.isArray(dstDescriptor.initializers) ? dstDescriptor.initializers : []);There was a problem hiding this comment.
I agree reduce is always better. This file is designed to be as readable as possible. I find reduce less readable than the forEach in this case.
There was a problem hiding this comment.
Yea I guess it's very subjective, I even think that the line with if (!Array.isArray(dstDescriptor.initializers)) is very hard to spot in there (especially without brackets) and it's almost unclear at a first glance why it's there. With reduce you clearly see how it's used and handled. But I am not going to fight you on this, if others like your way better too, it's fine.
There was a problem hiding this comment.
Yeah. The thing is too subjective. :)
Hey, do you want a commit access to the stampit-org? You'd be able to review and merge code. :)
UPD: I really need someone to merge this, and I trust your judged opinion.
There was a problem hiding this comment.
Well if it were up to me to merge I would not do it, because of my opinion above :)
There was a problem hiding this comment.
Fair enough. Let me change the code to the reduce implementation so that you can merge it. :)
There was a problem hiding this comment.
Ok. Done. See the latest commit.
Also, the invitation was sent. Feel free to accept and merge the PR (use the "squash and merge" github button pls).
Upgrade to the latest version of check-compose.
| srcDescriptor.initializers.forEach(i => { | ||
| if (isFunction(i) && dstDescriptor.initializers.indexOf(i) < 0) { | ||
| dstDescriptor.initializers.push(i); | ||
| dstDescriptor.initializers = srcDescriptor.initializers.reduce((result, init) => { |
There was a problem hiding this comment.
Well I hope you have tested it little, because I did not, I just wrote it from top of my head.
There was a problem hiding this comment.
I did test it. We have the special package check-compose which runs unit tests against a "compose()" implementation. I have just updated the check-compose up to version two, - added two more tests to check that initializers do not duplicate.
|
Ugh, there are actually some tests? Where are those defined? |
|
Ah I see, well sorry about |
|
Totally my bug. I should have checked it agains node 0.10 myself :) |
|
Hm, Node 0.10 is so old, if it would be up to me, I would go from 0.12 above :) |
|
Node 0.10 is too wide spread yet. We should deal with with. |
| if (!Array.isArray(dstDescriptor.initializers)) dstDescriptor.initializers = []; | ||
| dstDescriptor.initializers.push.apply(dstDescriptor.initializers, srcDescriptor.initializers.filter(isFunction)); | ||
| dstDescriptor.initializers = srcDescriptor.initializers.reduce((result, init) => { | ||
| if (isFunction(init) && result.indexOf(init) < 0) { |
There was a problem hiding this comment.
Why check < 0 instead of === -1? Just curious
There was a problem hiding this comment.
Using -1 always felt weird to be as some arbitrary number. You can even reason better about it like this: "index is below 0" instead of "index is equal this weird number".
There was a problem hiding this comment.
@unstoppablecarl shorter :)
There was a problem hiding this comment.
Makes sense. My instinct is to use === to be strict.
No description provided.