1413156 - Replace lodash usages with ES6 features (Part 1)

Status: RESOLVED FIXED

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

Description

[Ed Morley [:emorley]]

Now that we're using Babel (via Neutrino), we're able to use ES6 features. As such, some of the lodash functionality is now redundant and can be replaced with native ES6, reducing the amount of lodash that needs to be shipped to the end user (thanks to tree shaking, once bug 1384255 resolved).

For example the features mentioned here (and there may be more):
https://www.sitepoint.com/lodash-features-replace-es6/

For anyone working on this, I would:
1) Clone https://github.com/mozilla/treeherder
2) Get a local UI instance working by following the steps here: https://treeherder.readthedocs.io/ui/installation.html#running-the-standalone-development-server
3) Grep the `ui/` directory for lodash instances (eg: `_.map(...)`)
4) Replace them with ES6 equivalents using the guide above (or any other resources you can find)
5) Run the tests/linter locally
6) Check the local UI instance still works
7) Open a PR :-)

I would recommend tackling just one lodash feature at a time.

Comment 1

[GitHub Autolander Bot]

Attachment: PR 2902: Refactor _.filter to es6 array.filter

Comment 2

[GitHub Autolander Bot]

Attachment: [treeherder] CatEars:master > mozilla:master

Comment 3

[Henrik]

Attachment: PR 2921: Remove forEach usage for Arrays

Comment 4

[Ed Morley [:emorley]]

Comment on attachment 8925851 [details] [review]
[treeherder] CatEars:master > mozilla:master

Obsoleting in favour of the manually created attachment, since it contains the name of what's being updated, which will be handy if we end up with lots of linked PRs.

Henrik - thank you for the PR! As you might see above, there is someone already working on this bug, so you two might need to coordinate to avoid doubling up on work done. However given this bug is suited to being split up into mini-tasks, this might work well :-)

Comment 5

[jpumford]

Yeah, this is great! I can grab `_.map()` next (once I've heard back from @camd and how they would like to deal with lodash iterables). @Henrik just let me know what you're working on so I don't double up.

(In reply to Ed Morley [:emorley] from comment #4)
> Comment on attachment 8925851 [details] [review]
> [treeherder] CatEars:master > mozilla:master
> 
> Obsoleting in favour of the manually created attachment, since it contains
> the name of what's being updated, which will be handy if we end up with lots
> of linked PRs.
> 
> Henrik - thank you for the PR! As you might see above, there is someone
> already working on this bug, so you two might need to coordinate to avoid
> doubling up on work done. However given this bug is suited to being split up
> into mini-tasks, this might work well :-)

Comment 6

[Henrik]

(In reply to jpumford from comment #5)
> Yeah, this is great! I can grab `_.map()` next (once I've heard back from
> @camd and how they would like to deal with lodash iterables). @Henrik just
> let me know what you're working on so I don't double up.
> 
> (In reply to Ed Morley [:emorley] from comment #4)
> > Comment on attachment 8925851 [details] [review]
> > [treeherder] CatEars:master > mozilla:master
> > 
> > Obsoleting in favour of the manually created attachment, since it contains
> > the name of what's being updated, which will be handy if we end up with lots
> > of linked PRs.
> > 
> > Henrik - thank you for the PR! As you might see above, there is someone
> > already working on this bug, so you two might need to coordinate to avoid
> > doubling up on work done. However given this bug is suited to being split up
> > into mini-tasks, this might work well :-)

I found _.reduce somewhere when refactoring so I figured that could be my next one!

Lets finish this bug, 2x speed since we are two people! :)

Comment 7

[Cameron Dawson [:camd]]

Comment on attachment 8924425 [details] [review]
PR 2902: Refactor _.filter to es6 array.filter

Hit one error.  Please reassign to me when that's ironed out.  Thanks!!

Comment 8

[Ed Morley [:emorley]]

Comment on attachment 8924425 [details] [review]
PR 2902: Refactor _.filter to es6 array.filter

jpumford has updated the PR :-)

Comment 9

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/a0b606933b7fc1285397bfeaac0e3a3ab6177a21
Bug 1413156 - refactor _.filter to es6 array.filter

Remove existing lodash filter function and replace with
es6's array.filter(). Some whitespace changes were
made for clarity or because the linter got upset.

https://github.com/mozilla/treeherder/commit/797f4b55d3db0e3783febc128ff9fe6ea8afc2fc
Bug 1413156 - remove typographical error in migration to es6

When moving a _.filter() function to Array.filter(), an
"s" was left out throwing a type error. $scope.highlightedRevision
should have been $scope.highlightedRevisions

Comment 10

[GitHub Autolander Bot]

Attachment: PR 2957: fix linter 'array-callback-return' error

Comment 11

[Ed Morley [:emorley]]

Another handy guide:
https://github.com/you-dont-need/You-Dont-Need-Lodash-Underscore

Comment 12

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/7fb59420531cabb6ea8840a4911edd6028448c3e
Bug 1413156 - Fix eslint "array-callback-return" error (#2957)

Avoids an implicit return value of `undefined`, which would cause
one of the new eslint rules in bug 1364894 to fail.

Comment 13

[Cameron Dawson [:camd]]

Comment on attachment 8925853 [details] [review]
PR 2921: Remove forEach usage for Arrays

Most of these are great!  Just the one that's an object instead of an array.  Please reassign to me once you've got that issue resolved.

Thanks for the PR and sorry for how long it took me to get it reviewed.

Comment 14

[Ed Morley [:emorley]]

Comment on attachment 8925853 [details] [review]
PR 2921: Remove forEach usage for Arrays

Henrik updated the PR :-)

Comment 15

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/4b442b427c3d912625708ab854e89711322290fb
Bug 1413156 - Remove lodash forEach usage for arrays (#2921)

Comment 16

[Cameron Dawson [:camd]]

Comment on attachment 8925853 [details] [review]
PR 2921: Remove forEach usage for Arrays

Great job!  Thanks for the PR!

Comment 17

[GitHub Bugzilla PR Linker]

Attachment: PR 3016: Remove usages of lodash .reduce()

Comment 18

[Ed Morley [:emorley]]

Completely converted so far:
  _.filter

Partially converted:
  _.forEach

In progress:
  _.reduce (https://github.com/mozilla/treeherder/pull/3016)

Looking at master (using ripgrep), the following lodash usages are left:

treeherder $ rg -oN --no-filename '_\.[^(]+\(' -t js | sort | uniq -c
      3 _.apply(
      4 _.assign(
      1 _.at(
     18 _.bind(
      3 _.capitalize(
      6 _.chunk(
     12 _.clone(
      1 _.compact(
      3 _.countBy(
      4 _.defaults(
      1 _.defer(
      1 _.delay(
      3 _.difference(
     21 _.each(
      2 _.escape(
      6 _.every(
     10 _.extend(
      1 _.filter(
     77 _.find(
      2 _.findIndex(
      1 _.findLast(
      9 _.flatten(
      1 _.flowRight(
      8 _.forEach(
      5 _.forIn(
      2 _.get(
      2 _.groupBy(
     13 _.has(
     27 _.includes(
      1 _.indexOf(
      1 _.intersection(
      1 _.invert(
      2 _.isArray(
     14 _.isEmpty(
      6 _.isEqual(
      1 _.isPlainObject(
      1 _.isString(
     22 _.isUndefined(
      1 _.keyBy(
     10 _.keys(
      2 _.last(
     87 _.map(
      1 _.mapKeys(
      1 _.mapValues(
      6 _.max(
      1 _.min(
      1 _.omit(
      1 _.padEnd(
      1 _.padStart(
      2 _.pick(
      1 _.pickBy(
      1 _.pop(
      1 _.push(
      3 _.reduce(
      1 _.reject(
      3 _.remove(
      1 _.result(
      2 _.set(
     11 _.size(
      9 _.some(
      8 _.sortBy(
      2 _.sum(
      1 _.unescape(
     12 _.union(
     33 _.uniq(
      5 _.values(
      3 _.without(
      1 _.zipObject(

Cross-referencing that list against these:
https://www.sitepoint.com/lodash-features-replace-es6/
https://github.com/you-dont-need/You-Dont-Need-Lodash-Underscore#quick-links

...finds the following lodash features in use that might be suitable for conversion:
_.assign(
_.compact(
_.each(
_.every(
_.filter(
_.find(
_.findIndex(
_.flatten(
_.groupBy(
_.includes(
_.indexOf(
_.isArray(
_.keys(
_.last(
_.map(
_.pick(
_.reduce(
_.size(
_.some(
_.values(
_.without(

Comment 19

[Ed Morley [:emorley]]

Attachment: PR 3154: includes, indexOf & compact

Cameron, I don't suppose you'd mind taking a look at this one? :-)

Comment 20

[Ed Morley [:emorley]]

Comment on attachment 8944559 [details] [review]
PR 3154: includes, indexOf & compact

https://github.com/mozilla/treeherder/commit/bcb4011bb96e1fc258d86f784dd1da25ef32e03c

Comment 21

[GitHub Bugzilla PR Linker]

Attachment: PR 3175: Fix inverted conditional in renderGroupJobsAndCounts

Comment 22

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/d0bfec4c9ea1d3e9cdcadd5872b9c78f2fdacd0e
Bug 1413156 - Fix inverted conditional in renderGroupJobsAndCounts (#3175)

Prior to #3154 the conditional used:
`_.includes(failResults, resultStatus)`

...so should now be using `!== -1` rather than `=== 1`.

Comment 23

[Ed Morley [:emorley]]

Clearing the assignee since multiple people are working on this, and there are still parts of for grabs if others come along :-)

Comment 24

[Anubhav Tiwari]

I'm Working on it as well, i've fixed 4 files for all instances of lodash till now, and 've found all instances of lodash and i'll submit a PR once i've finished.
Just wanted to know how to implement chunks?

Comment 25

[Anubhav Tiwari]

I mean, i know the logic, but should i directly plug in the code or should i right a separate function?

Comment 26

[Ed Morley [:emorley]]

I guess it depends on how many times it's repeated. Perhaps use your judgement and we can iterate once the PR is open? :-)

Comment 27

[GitHub Bugzilla PR Linker]

Attachment: PR 3332: _.size to .length

Comment 28

[Jenny Seo]

https://github.com/mozilla/treeherder/pull/3332 -  Bug 1413156 - lodash to ES6: _.size to .length
https://github.com/mozilla/treeherder/pull/3338 -  Bug1413156 - lodash to ES6: replace _.without _.values _.last usage

Hello, I've converted the following methods: _.size, _.without, _.values, and _.last. Please let me know if any changes are required =).

Comment 29

[Ed Morley [:emorley]]

Attachment: PR 3338: replace _.without _.values _.last

Comment 30

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/31dc8dec9918b19fccad8054fc47a784dec8cfcc
Bug 1413156 - lodash to ES6: _.size to .length (#3332)

Comment 31

[Cameron Dawson [:camd]]

Comment on attachment 8958075 [details] [review]
PR 3332: _.size to .length

Thanks!  Great work.  :)

Comment 32

[Cameron Dawson [:camd]]

Comment on attachment 8959112 [details] [review]
PR 3338: replace _.without _.values _.last

I've requested a few changes here.  Please ping me in IRC when you've pushed your changes.  Thanks for the PR!  :)

Comment 33

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/5f68792a587f312b7ec847837b0f98fd8989c66b
Bug1413156 - lodash to ES6: replace _.without _.values _.last usage (#3338)

Comment 34

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/cc7411e3525ff7d0521be548b7845c5669ac9e38
Bug1413156 - lodash to ES6: replace _.reduce, _.assign, _.flatten, _.isArray (#3357)

Comment 35

[GitHub Bugzilla PR Linker]

Attachment: PR 3374: replace _.some and _.keys in PerfHerder

Comment 36

[Cameron Dawson [:camd]]

Comment on attachment 8962252 [details] [review]
PR 3374: replace _.some and _.keys in PerfHerder

Need a couple small changes to this.  Thanks for the PR!!  :)

Comment 37

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/9c017283c7b26cad4850cfd44927af8a3f974663
Bug 1413156 - lodash to ES6: replace _.some, _.keys in PerfHerder (#3374)

Comment 38

[GitHub Bugzilla PR Linker]

Attachment: PR 3406: Remove _.forEach() usage for objects

Comment 39

[Simon]

Hi, would like to make some (my very first)contributions. To make sure everything runs smoothly I'll start with a small PR.

https://github.com/mozilla/treeherder/pull/3406

Let me know if anything seems incorrect :)

Comment 40

[Cameron Dawson [:camd]]

Requested a fix for this change in the PR.  Thanks!

Comment 41

[Simon]

(In reply to Cameron Dawson [:camd] from comment #40)
> Requested a fix for this change in the PR.  Thanks!

Thanks for reviewing! You're right about the missed forEach(). One question - is the norm when making small changes to the PR to push a new commit to the branch or to make an update to the last commit? (I did the later thing this time as you can see)

Comment 42

[Cameron Dawson [:camd]]

Simon--  Either one is fine.  I somewhat prefer an additional commit, so the path you took was great. :)  But it's not a huge deal to me.  Github allows you to see some of what things looked like when I made my comment, even when squashed.

I'll take a look shortly!  :)

Comment 43

[Cameron Dawson [:camd]]

Comment on attachment 8965365 [details] [review]
PR 3406: Remove _.forEach() usage for objects

Thanks for the PR!

Comment 44

[Ed Morley [:emorley]]

Comment on attachment 8965365 [details] [review]
PR 3406: Remove _.forEach() usage for objects

https://github.com/mozilla/treeherder/commit/501179c450b7c6f6c8c2e07382f5975ea2da72e6

(The commit message was missing the bug number)

Comment 45

[Simon]

(In reply to Cameron Dawson [:camd] from comment #43)
> Comment on attachment 8965365 [details] [review]
> PR 3406: Remove _.forEach() usage for objects
> 
> Thanks for the PR!

Thanks for accepting the PR! :)

(In reply to Ed Morley [:emorley] from comment #44)
> Comment on attachment 8965365 [details] [review]
> PR 3406: Remove _.forEach() usage for objects
> 
> https://github.com/mozilla/treeherder/commit/
> 501179c450b7c6f6c8c2e07382f5975ea2da72e6
> 
> (The commit message was missing the bug number)

Will keep that in mind for the future!

Comment 46

[Simon]

Have converted the remaining lodash .forEach functions operating on objects.

https://github.com/mozilla/treeherder/pull/3438

I'll follow the advice from earlier in the thread of tackling one(or a few) lodash features at a time.

Comment 47

[Ed Morley [:emorley]]

Attachment: PR 3448: Remove _.forEach() usage for objects (part 2)

Comment 48

[Ed Morley [:emorley]]

Attachment: PR 3391: Replace _.map() in several perfherder files

Comment 49

[Cameron Dawson [:camd]]

PR 3448 Needs fixups, but leaving assigned to me because PR author doesn't have privs to assign to me when it's fixed up.

Comment 50

[Cameron Dawson [:camd]]

PR 3391 also needed fixups, but leaving assigned to me for same reason

Comment 51

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/53fb77ee8daa2b169cdac2eac84036f6e4d1e2f3
Bug 1413156 - lodash to ES6: replace _.map in components/perf/compare.… (#3391)

Comment 52

[Cameron Dawson [:camd]]

Comment on attachment 8968069 [details] [review]
PR 3448: Remove _.forEach() usage for objects (part 2)

I don't want to leave this in my review queue while I'm on PTO.  So marking r- for now until we receive updates.  I'll keep an eye on my email to see when those updates are made.  :)

Comment 53

[GitHub Bugzilla PR Linker]

Attachment: PR 3537: Replace _.findIndex()

Comment 54

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/be3c6e8bc9caf31584196fec40d14d243adb4a39
Bug 1413156 - Convert from lodash findIndex to native ES6 (#3537)

Comment 55

[Ed Morley [:emorley]]

Attachment: PR 3536: Replace _.bind()

Comment 56

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/dc2a2e29777f2aa599b34ee3d7b1b307e650e55b
Bug 1413156 - Convert from lodash bind to native ES6 (#3536)

Comment 57

[aamarill.engr]

Hello,

I am looking to make my first open source contribution. Is this a good issue to take on?
If not, feel free to point me other issues that might be good for someone that's new to open source.
Thanks!

Comment 58

[Ed Morley [:emorley]]

We'll happily accept contributions to this issue - I think it should be a good first bug. Comment 18 has some ideas for which parts to work on next (lots of people have been working on this bug, helping with one piece at a time) :-)

Comment 59

[lucas05]

Can I work on this? It would be my first contribution :)

Comment 60

[Alok Pant]

Hey, looks like _.without() has not been picked up yet. Can I get involved on it as my first contribution?

Comment 61

[Cameron Dawson [:camd]]

Alok and lucas05:  Absolutely!  Please submit a PR when it is ready and need-info me in this bug.  Thanks!

Comment 62

[Ed Morley [:emorley]]

This bug has gotten a bit long / hard to follow, so I've filed a new meta bug for the remaining work here - where we will use a separate dependant bug for each PR, to make things more manageable.

If anyone would like to continue working on this, please see the revised instructions over there:
https://bugzilla.mozilla.org/show_bug.cgi?id=1466494#c0

Comment 63

[Ed Morley [:emorley]]

Comment on attachment 8934532 [details] [review]
PR 3016: Remove usages of lodash .reduce()

This PR was superseded by:
https://github.com/mozilla/treeherder/pull/3357

Comment 64

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/5c51ee975d30163a18a29566c3c22a7191ad8287
Bug 1413156 - Remove lodash forEach usage for objects (#3438)