Style Guide suggestion: lodash

[stacey-gammon]

Chatted with @w33ble about this a bit today and I'd like to confirm whether we should be moving towards lodash (as I guess it is in our current style guide?) or away from it. Perhaps we can make a general style guide rule around this.

Edit @epixa: We should update the styleguide to encourage people to use native methods instead of their lodash counterparts wherever possible.

[w33ble]

The only current mention I see where using lodash is mentioned (and slightly discouraged) is in iterating objects and arrays.

Generally speaking, I'd like to see the styleguide discourage the use a lodash where native functionality exists. One example: there are so many places where _.assign is used, instead of the native Object.assign, even in recently written code. There's no need for this.

I'm in favor of making exceptions for real helpers, like _.get and _.set, but I'd prefer to see that functionality come from specialized modules instead of the kitchen sink lodash.

[cjcenizal]

I agree with the idea of moving away from lodash in favor of native methods, wherever possible. We should solve the same problems consistently in the same way, to improve the predictability and readability of our code.

We've been floating the idea around of creating a service that exposes proxy methods to the lodash methods which are actually useful. Such a service would help us control our usage of lodash, and make it easier to migrate away from it if/when the need arises.

[cjcenizal]

If we do decide to move away from lodash, maybe we should make a decision on #7537 too? That issue requests we upgrade lodash. Instead of upgrading, we should probably selectively pull in the helpers that we need.

[w33ble]

WRT #7537, moving more of our code to native calls would make upgrading a whole lot easier, since there's way less breaking changes that would affect us. Maybe instead of updating the code to support lodash4, we should update it to use native calls and lodash4.

[Bargs]

I don't think we can make a blanket statement favoring native methods. Many of lodash's methods can greatly simplify code and make it less brittle by allowing you to ignore the type of the target object. It encourages you to think in terms of functions and data structures rather than objects and methods.

[w33ble]

Many of lodash's methods can greatly simplify code and make it less brittle by allowing you to ignore the type of the target object

Are you talking about the Collection methods in lodash?

My gut reaction is that if you don't know the type of Object you are dealing with (Object vs Array), then there's probably larger problems. But I'd be curious to see examples where it's been useful so I can get some more perspective.

[Bargs]

Yes, mainly the collection methods, though there are others that are categorized under a particular type but work equally well with arrays and objects, like get and transform.

Flattening an object from elasticsearch would be a great example where lodash allows you to completely ignore the difference between arrays and objects ;)

Arrays are objects, saying that it's a problem just because you don't know (or care) which you're dealing with is essentially saying that polymorphism is a problem.

And TBH I think using lodash leads to more consistency. Why can't I .map an object? Because it's on the Array prototype. Ugh. Thanks to that, we have to use two totally different mechanisms for mapping objects and arrays if we insist on using native methods where available. Now you've doubled the size and complexity of any code that has to deal with both arrays and objects.

[cjcenizal]

I would also like to see some examples to broaden my perspective. Matt's ES object example sounds interesting, and an example of how code dealing with both arrays and objects gets doubled in size and complexity would be helpful.

Speaking for myself, I like to know the data structure I'm operating on, because I'm used to using arrays and objects for different purposes (storing sequential data vs associative data). What are the benefits to not knowing?

[thomasneirynck]

fwiw, I'm in agreement with @w33ble, @cjcenizal. I generally prefer code that is as precisely typed as possible, which means not abstracting away differences between Object/Array when not strictly necessary. Also, if native methods exist, I prefer using those over lodash too, as that removes a layer of indirection.

[spalger]

I'm with @w33ble, @cjcenizal and @thomasneirynck on this one.

In my experience code using the lodash collection functions rarely need, or want, the flexibility that they provide. I think this flexibility is a lot more likely to lead to misuse or confusion about the intention of a function/module.

That said, there is no reason we couldn't expose similar helper methods that make that intention very clear.

Consider this super contrived example:

// Extract the odd values from a list of numbers
function getOddNumbers(values) {
  return _.filter(values, x => x % 2)
}

The comment above this function sort of alludes to the expectation that values is an array, and that is use case the unit tests check, but it is also compatible with objects.

Now, Imagine I was obsessed about performance and wanted to avoid every possible function call. If I saw this code and wanted to avoid calling the "expensive" _.filter function I would, without a doubt, convert the code to be:

function getOddNumbers(values) {
  if (!values.length) {
    return []
  }

  return _.filter(values, x => x % 2)
}

Obviously, this code no longer supports the scenario where values is an object, but the tests pass and everything is great. Unfortunately, there is an obscure component somewhere that uses this function to pull the odd numbers out of an object (it might not even realize it's an object) and that code just started receiving an empty array.

What if the original code was instead:

function getOddNumbers(values) {
  return values.filter(x => x % 2)
}

Maybe the obscure component would have changed it to this in order to support its new use case:

import { filterArrayOrObject } from '../lib'
function getOddNumbers(values) {
  return filterArrayOrObject(values, x => x % 2)
}

I feel confident that performance-obsessed-spencer would approach this problem differently, and probably would have given up (calling Object.keys(values).length before iterating is against his morals).

---

Like I said, super contrived example, but I think it illustrates a concept that definitely scales to much larger and more complicated functions/problems. The addition of the .length check could be any modification to a method using the lodash collection functions.

While these functions make the code more flexible, I think they also make it harder to define which I think leads to more bugs.

[Bargs]

@spalger your example is an argument for pre condition checks, better
documentation, and careful API design, not usage of a particular
iteration method. If a function is only intended to work with arrays, it
should explicitly test its params at the beginning of the function, have
adversarial unit tests, and clearly document its param types. This is
significantly more robust than relying on whatever error .filter happens
to return and it clearly conveys the author's intention.

I'm not arguing that lodash methods are always the right tool. But
sometimes they are and it would be silly to make a blanket statement
against using them.
On Thu, Nov 17, 2016 at 3:57 AM Spencer [email protected] wrote:

I'm with @w33ble https://github.com/w33ble, @cjcenizal
https://github.com/cjcenizal and @thomasneirynck
https://github.com/thomasneirynck on this one.

In my experience code using the lodash collection functions rarely need,
or want, the flexibility that they provide. I think this flexibility is a
lot more likely to lead to misuse or confusion about the intention of a
function/module.

That said, there is no reason we couldn't expose similar helper methods
that make that intention very clear.

Consider this super contrived example:

// Extract the odd values from a list of numbersfunction getOddNumbers(values) {
return _.filter(values, x => x % 2)
}

The comment above this function sort of alludes to the expectation that
values is an array, and that is use case the unit tests check, but it is
also compatible with objects.

Now, Imagine I was obsessed about performance and wanted to avoid every
possible function call. If I saw this code and wanted to avoid calling the
"expensive" _.filter function I would, without a doubt, convert the code
to be:

function getOddNumbers(values) {
if (!values.length) {
return []
}

return _.filter(values, x => x % 2)
}

Obviously, this code no longer supports the scenario where values is an
object, but the tests pass and everything is great. Unfortunately, there is
an obscure component somewhere that uses this function to pull the odd
numbers out of an object (it might not even realize it's an object) and
that code just started receiving an empty array.

What if the original code was instead:

function getOddNumbers(values) {
return values.filter(x => x % 2)
}

Maybe the obscure component would have changed it to this in order to
support its new use case:

import { filterArrayOrObject } from '../lib'function getOddNumbers(values) {
return filterArrayOrObject(values, x => x % 2)
}

I feel confident that performance-obsessed-spencer would approach this
problem differently, and probably would have given up (calling

Object.keys(values).length before iterating is against his morals).

Like I said, super contrived example, but I think it illustrates a concept
that definitely scales to much larger and more complicated
functions/problems. The addition of the .length check could be any
modification to a method using the lodash collection functions.

While these functions make the code more flexible, I think they also make
it harder to define which I think leads to more bugs.

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#9079 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF8zyGP9lhxZGmsvhLEAFk-Wpg5eyjZDks5q_BbrgaJpZM4Kyz4_
.

[cjcenizal]

Good code design includes everything @Bargs mentioned, but in this case I think it's clear that using _.filter doesn't offer any advantage over 'Array.filter' if the function rejects non-arrays. I think this is a good example of what I originally meant by favoring native methods over lodash. If there's a native method that fulfills a need, let's use it. If native methods don't get the job done, bring out the big guns and use lodash.

We could use this as a guide: https://github.com/you-dont-need/You-Dont-Need-Lodash-Underscore

[w33ble]

I'm not arguing that lodash methods are always the right tool. But sometimes they are and it would be silly to make a blanket statement against using them.

You're right. I don't think anyone is explicitly calling for the removal of all of lodash[1], but for a preference for native functonality. I'd personally like to see the styleguide be very explicit about that, and if you're using lodash for things like filter, map, reduce, and other clearly native methods, you better have a really good explanation for that choice come review time.

[1] like I said before though, I'd like to see really useful methods (get/set being obvious candidates) become their own modules, or come from more specialized external modules, but that's a different conversation

[Bargs]

I don't particularly mind using native methods where they make sense. But let me play devil's advocate. What's the benefit? The guide @cjcenizal linked to only offers this one piece of motivation:

If you want your project to require fewer dependencies, and you know your target browser clearly, then you may not need Lodash/Underscore.

Lodash is one of the last of our 92 dependencies that I can imagine us wanting to get rid of.

The other argument I heard was for consistency:

We should solve the same problems consistently in the same way, to improve the predictability and readability of our code.

As I already said, I think preferring native methods makes code more inconsistent. Here's an example:

Problem: map an iterable object
Lodash solution: Use _.map
Native solution: Use .map if the type you're working with supports it. Otherwise invent your own solution or use _.map.

Which one will lead to more consistency? It depends on how you frame the question and lodash frames it in terms of interfaces, not concrete types. This isn't some crazy reckless idea born from javascript's dynamic typing, Java has the same thing. Generic interfaces are a good thing.

I just don't see an objective benefit in favoring native methods here. We have so much real tech debt it feels like we're barking up the wrong tree.

[cjcenizal]

Well, speaking for myself, I generally reach for the language's features before I look for a library. I feel that, since I'm writing JavaScript, I should try to use JavaScript to solve a problem and communicate my intent.

Lodash provides 323 methods, most of which I need to refer to the docs to use. Searching for _. in our code shows some hits which are unclear to me off the bat: _.flow(), _.compact(), _.restParam().

Even ones which seem clear to me require me to check the docs, because the behavior is a little unusual. For example, I assumed _.size() is equivalent to checking the length of an array, but the docs state that you can use it on objects as well, to get the "number of own enumerable string keyed properties". If we're checking an object for the number of own enumerable string keyed properties, I'd like that to be a little more obvious in the code, i.e. get the object's keys and check the length.

Anyway, I'm just one data point. But to answer your question: if other developers reading our code feel similarly to the way I do, then a benefit to preferring native methods is that we're making our code more accessible to more developers.

[stacey-gammon]

+1 to CJ in that some of the lodash functions are not immediately clear to
me, and I end up having to spend time looking them up.

On Thu, Nov 17, 2016 at 2:08 PM, CJ Cenizal [email protected]
wrote:

Well, speaking for myself, I generally reach for the language's features
before I look for a library. I feel that, since I'm writing JavaScript, I
should try to use JavaScript to solve a problem and communicate my intent.

Lodash provides 323 methods, most of which I need to refer to the docs to
use. Searching for _. in our code shows some hits which are unclear to me
off the bat: _.flow(), _.compact(), _.restParam().

Even ones which seem clear to me require me to check the docs, because the
behavior is a little unusual. For example, I assumed _.size() is
equivalent to checking the length of an array, but the docs state that you
can use it on objects as well, to get the "number of own enumerable string
keyed properties". If we're checking an object for the number of own
enumerable string keyed properties, I'd like that to be a little more
obvious in the code.

Anyway, I'm just one data point. But to answer your question: if other
developers reading our code feel similarly to the way I do, then a benefit
to preferring favorite native methods is that we're making our code more
accessible to more developers.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#9079 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/APy9kyZC7mlLWXkrbdFbS0z45xkBEIVjks5q_KY4gaJpZM4Kyz4_
.

[w33ble]

But let me play devil's advocate. What's the benefit?

My reasoning is that the less we use lodash, the easier it is to keep it updated. Looking at the breaking changes in lodash4, it doesn't look like they'll affect us, but without digging a lot deeper, I'm really not sure. And since we don't have 100% test coverage, we can't really know for sure unless we actually look through the whole codebase. Lodash is just one example, this applies to really any external dependency we're using.

That's also the reason I'm in favor of using smaller, more focused helpers to get the functionality we're looking for instead of a "kitchen sink" library. The size of lodash makes it really hard to know what we're using and what breaking changes we're affected by. Smaller modules should make usage easier to find and thus easier to upgrade, and they should get upgrades much less often as they become stable.

[epixa]

When it comes to dependencies, we should be justifying their existence rather than justifying not using them at all. I agree with most of the feedback here that we should prefer native methods to their alternative lodash implementations, and that's easy enough to add to the styleguide.

Whether we need lodash at all is a reasonable question, but we should treat that as a separate issue than this, and the first step toward seriously considering that option would need to be an audit of the codebase to help summarize how we're actually using lodash rather than trying to make a decision based on how we each think we're using lodash.

It's not even a question of whether lodash is valuable as a library, it's a question of whether we're actually using it in valuable ways that don't have a simpler solution.

[w33ble]

I agree with most of the feedback here that we should prefer native methods to their alternative lodash implementations, and that's easy enough to add to the styleguide.

Is that the plan then, adding a preference for native methods to the styleguide?

[epixa]

@w33ble Yeah, let's do that

Edit: I updated the PR description

[stacey-gammon]

Final decision was: if you can use native code instead of lodash, do so, but lodash is allowed if it makes something more readable or more concise, the definition of which is up to the coder and reviewers.

Closing as this is no longer an active discussion.