Don't automatically combine already combined topics #43
Conversation
src/notifications.js
Outdated
| topics.map(t => { | ||
| const precombinedTopics = []; | ||
| topics.filter(t => { | ||
| if (t.indexOf('?') === -1) { |
There was a problem hiding this comment.
Can use !t.includes('?') now that we don't have to support IE
There was a problem hiding this comment.
Okay--I since this is a public project I thought maybe we still wanted to try to be compatible, but I'm certainly happy to use includes instead :)
There was a problem hiding this comment.
True, though I'm willing to say dropping IE support extends to this library as well, I'm cool either way 👍
src/notifications.js
Outdated
| if (t.indexOf('?') === -1) { | ||
| return true; | ||
| } else { | ||
| precombinedTopics.push(t); |
There was a problem hiding this comment.
Might be good to push { id: t } here rather than the additional map below for consistency with the other push call to combinedTopics
src/notifications.js
Outdated
| if (t.indexOf('?') === -1) { | ||
| return true; | ||
| } else { | ||
| precombinedTopics.push(t); |
There was a problem hiding this comment.
Also, this is a side-effect and should generally be avoided. By that, I mean you shouldn't do
something.filter((item) => {
if (!item.condition) {
// do something unrelated
}
return item.condition
}
I understand why you are doing this and that it saves a line or two of code, but I'd prefer either using a forEach to create 2 different arrays, or running a separate dedicated filter to find the precombinedTopics. Does that make sense what I'm saying?
There was a problem hiding this comment.
Yep, it does, and will do!
| @@ -370,3 +370,26 @@ test('notifications | mapCompineTopics should correctly reduce topics', t => { | |||
| t.is(reducedTopics[1].id, 'v2.users.testuser.thisIsAReallyLongTopicForThePurposeOfExceeding200CharsinCombinedTopicNames'); | |||
| t.is(reducedTopics[2].id, 'v2.users.testuser?InRealityTheseWouldBeALotOfDisparateTopicsThatWhenJoinedExceed200Chars&athirdreallylongtopicathirdreallylongtopicathird'); | |||
| }); | |||
|
|
|||
| test('notifications | mapCompineTopics should not combine already combined topics', t => { | |||
No description provided.