1467401 - Convert lodash .groupBy() to native ES6 JS

Status: RESOLVED FIXED

(Tree Management :: Treeherder: Frontend, defect, P3)

Description

[Eliott Vincent [:evct] (GMT+2)]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3359.170 Safari/537.36 OPR/53.0.2907.68

Steps to reproduce:

Converting all _.groupBy() (lodash) functions to native .filter() (ES6 JS)

Comment 1

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/3629

Comment 2

[Eliott Vincent [:evct] (GMT+2)]

Typo in my original comment: "Converting all _.groupBy() (lodash) functions to native ES6 JS".

(I don't know how to edit comment)

Comment 3

[Ed Morley [:emorley]]

Unfortunately Bugzilla doesn't support editing comments at the moment (bug 540) - the typo doesn't matter too much since it's mainly the bug title and PR/commit titles that will be read.

Unrelated, but myself and Cameron have this Bugzilla component in our watch list, so adding the CCs isn't needed, if you'd rather save time by skipping that step :-)

Comment 4

[Eliott Vincent [:evct] (GMT+2)]

Ok thanks for the information, and I will skip this step in the future

Comment 5

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/899396651714e9454946396ecd124db128fabc8d
Bug 1467401 - convert _.groupBy() usages to ES6 (#3629)

Comment 6

[Ed Morley [:emorley]]

When I visit stage (https://treeherder.allizom.org/#/jobs?repo=mozilla-inbound) I get:

TypeError: "R.repos is undefined"
getOrderedRepoGroups https://treeherder.allizom.org/index.593ea103062eb159bdc8.bundle.js:31:7795
fn https://treeherder.allizom.org/vendor.bf21b3a1381cc2dcd328.bundle.js line 45 > Function:4:153
...

...which I think is from:
https://github.com/mozilla/treeherder/commit/899396651714e9454946396ecd124db128fabc8d#diff-46e683ff6ffab8c2ca1bdf9d1bd2bb00R36

The ES6 equivalent for _.groupBy() is also pretty verbose. I'm wondering if we should leave that one as is for now, or even see if we can adjust the repo groups data structure so it doesn't need such transformation?

Comment 7

[Eliott Vincent [:evct] (GMT+2)]

Ok I totally missed this error but indeed I'm having the same behavior on my side... Looking into it.

I'm not used to the code base and the various data structure of Treeherder yet, but that may be a good idea.

Comment 8

[Eliott Vincent [:evct] (GMT+2)]

In fact `$rootScope.repos` is not filled instantly (slight loading delay I guess), so the very first calls of `getOrderedRepoGroups()` won't do anything since `$rootScope.repos` will be set as undefined.
Then the next calls of getOrderedRepoGroups() actually work because `$rootScope.repos` has been filled with the repos.

_.groupBy() ignored this case, while the ES6 equivalent I wrote fails if the collection on which we apply .reduce() is undefined.

A possible solution could be to first check if `$rootScope.repos` is not undefined. I can submit a patch if it's ok with you.

Comment 9

[Cameron Dawson [:camd]]

Dang, my sincere apologies for not catching this.  I must have neglected to have the console window open when testing this.

Ed's point about restructuring the repos structure.  I'm happy to look into doing this when I convert the nav bars to React.

I'll revert this for now.

Comment 10

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/3643

Comment 11

[Ed Morley [:emorley]]

Comment on attachment 8984698 [details] [review]
Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/3643

(Obsoleting since bug 1468533 is taking a different approach)