1468160 - Convert lodash .max() to native ES6 JS

Status: RESOLVED FIXED

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

Description

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

Converting all _.max() (lodash) functions to Math.max() (ES6 JS)

Comment 1

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

I plan on taking advantage of the spreading operator to use Math.max():

const arr = [2, 3, 8];
const max = Math.max( ...arr );
console.log(max);

Comment 2

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

Ok looks like _.max() is mostly used to compare Date objects... 
Math.max() is going to be useless here. Have to reconsider this one and Bug 1468161.

Comment 3

[GitHub Bugzilla PR Linker]

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

Comment 4

[Sarat Addepalli [:SirR4T]]

(In reply to Eliott Vincent [:evct] (GMT+2) from comment #2)
> Ok looks like _.max() is mostly used to compare Date objects... 
> Math.max() is going to be useless here. Have to reconsider this one and Bug
> 1468161.

I've resorted to doing the below:

    [dateObj1, dateObj2, dateObj3].reduce((a, b) => (a > b ? a : b))


in place of:

    _.max([dateObj1, dateObj2, dateObj3])

Comment 5

[Sarat Addepalli [:SirR4T]]

Needs review. Commenting here, since the "details" section doesn't show me any suggested reviewers.

Comment 6

[Sarat Addepalli [:SirR4T]]

Hi Ed, I could see that you marked the GitHub PR for review by :camd, but i was wondering why the Bugzilla bug's review flag wasn't updated. Hope that will not be an issue in workflows for anyone.

Comment 7

[Cameron Dawson [:camd]]

Awaiting rebase to resolve conflicts created when other PRs were merged.

Comment 8

[Sarat Addepalli [:SirR4T]]

Fixed conflicts after rebase, and pushed now.

Comment 9

[Cameron Dawson [:camd]]

Thanks for the updates.  I'll review this on Friday.

Comment 10

[Cameron Dawson [:camd]]

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

Asked for changes in the PR.

Comment 11

[Cameron Dawson [:camd]]

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

Reassigning to myself awaiting requested changes.

Comment 12

[Cameron Dawson [:camd]]

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

Unassigning for now till there are updates.  Sirat, please comment in the PR when you've made the changes.  Thanks!!  :)

Comment 13

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/d0afcc7cf21490dac1af0dfe9d2de6c098091e50
Bug 1468160 - import max from lodash

https://github.com/mozilla/treeherder/commit/92f3660a8498a3e2095996c1e587d1e30712ca30
Revert "Bug 1468160 - import max from lodash"

This reverts commit 029cec1673ac127dbaea5cf49226764d7d218ba3.

https://github.com/mozilla/treeherder/commit/ceed244ffbf1cf0f196473dc248f2f06d3ad42a9
Bug 1468160 - Remove occurences of _.max

- Revert previous attempt
- Import `max` from `lodash/max` wherever readability is affected
- Use `Math.max(...resultArray)` wherever applicable
- Remove unnecessary usage when comparing only two elements
- Remove unnecessary `import _ from 'lodash'`

https://github.com/mozilla/treeherder/commit/5b41733d100568375107f300dd4a5373a0024fba
Bug 1468160 - correct the usage of lodash max

Comment 14

[Cameron Dawson [:camd]]

Oh shoot, I accidentally merged the changes instead of squashing them.  Ugh, sorry for the commit noise...  :(

Comment 15

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/af7e94cfdb996ab4f76c93bd1f0c893ab357e19b
Bug 1468160 - Convert _.max to use native ES6 JS (#3858)

Two instances have been left using lodash for now, since `Math.max()`
does not preserve `Date()` objects unlike lodash. These cases have
been converted to use the tree-shaking compatible import form.

Comment 16

[Ed Morley [:emorley]]

I've squashed the commits on master and force pushed to update, since nothing else had been committed since :-)